Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add deprecation warning for ITPParser #4744

Merged
merged 8 commits into from
Oct 24, 2024
Merged

Add deprecation warning for ITPParser #4744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d59d8f5
add deprecation warning for ITPParser
lilyminium Oct 19, 2024
464a6ab
make warning a little more verbose
lilyminium Oct 19, 2024
0cbf2d4
add test for no deprecation warning in itp without valid elements
lilyminium Oct 19, 2024
ba37e13
fix pep8
lilyminium Oct 19, 2024
578af3f
Update ITPParser.py
lilyminium Oct 19, 2024
095d58a
Update test_itp.py
lilyminium Oct 19, 2024
c07e7e6
Update test_itp.py
lilyminium Oct 19, 2024
64804b6
Apply suggestions from code review
IAlibay Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package/CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ Changes
numpy.testing.assert_allclose #4438)

Deprecations
* Element guessing in the ITPParser is deprecated and will be removed in version 3.0
(Issue #4698)
* Unknown masses are set to 0.0 for current version, this will be depracated
in version 3.0.0 and replaced by :class:`Masses`' no_value_label attribute(np.nan)
(PR #3753)
Expand Down
10 changes: 9 additions & 1 deletion package/MDAnalysis/topology/ITPParser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,15 @@ def parse(self, include_dir='/usr/local/gromacs/share/gromacs/top/',
if all(e.capitalize() in SYMB2Z for e in self.elements):
attrs.append(Elements(np.array(self.elements,
dtype=object), guessed=True))

warnings.warn(
"The elements attribute has been populated by guessing "
"elements from atom types. This behaviour has been "
"temporarily added to the ITPParser as we transition "
"to the new guessing API. "
"This behavior will be removed in release 3.0. "
"Please see issue #4698 for more information. ",
DeprecationWarning
)
else:
warnings.warn("Element information is missing, elements attribute "
"will not be populated. If needed these can be "
Expand Down
17 changes: 17 additions & 0 deletions testsuite/MDAnalysisTests/topology/test_itp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,20 @@ def test_missing_elements_no_attribute():
u = mda.Universe(ITP_atomtypes)
with pytest.raises(AttributeError):
_ = u.atoms.elements


def test_elements_deprecation_warning():
"""Test deprecation warning is present"""
with pytest.warns(DeprecationWarning, match="removed in release 3.0"):
mda.Universe(ITP_nomass)


def test_elements_nodeprecation_warning():
"""Test deprecation warning is not present if elements isn't guessed"""
with pytest.warns(UserWarning) as record:
mda.Universe(ITP_atomtypes)
assert len(record) == 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best to not do this - i.e. we know this is prone to failures as soon as someone adds another warning somewhere else. However it's best to just fix it later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also just use no_deprecated_call context manager I wrote for MDA 7 years ago in gh-1522.

Copy link
Member Author

@lilyminium lilyminium Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tyler -- trying the below code raised an error for me that I won't have time to look at for a few hours. Any quick suggestions? Otherwise since @IAlibay has called the feature freeze for his Sunday, I can raise an issue and fix this afterwards as he suggested.

def test_elements_nodeprecation_warning():
    "Test deprecation warning is not present if elements isn't guessed"
    with no_deprecated_call():
        mda.Universe(ITP_atomtypes)

raises

../util.py:244: in __exit__
    if any(issubclass(c, deprecation_categories) for c in self._captured_categories):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

.0 = <list_iterator object at 0x12cc235b0>

>   if any(issubclass(c, deprecation_categories) for c in self._captured_categories):
E   TypeError: issubclass() arg 1 must be a class

../util.py:244: TypeError

Edit: ah, the captured categories is a list of [None, None]. I had thought warnings.warn defaulted to UserWarning but maybe I'm confused here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, analysis below, maybe I'll open a ticket about this since I think I agree with you that the explicit requirement to add UserWarning shouldn't be needed (though it does fix the problem). Perhaps this is some subtlety about Python 2 at the time vs. 3 now:

this works just fine:

def test_elements_nodeprecation_warning():
    "Test deprecation warning is not present if elements isn't guessed"
    def foo():
        warnings.warn("boo", UserWarning)
    with no_deprecated_call():
        foo()

this fails like your report:

def test_elements_nodeprecation_warning():
    "Test deprecation warning is not present if elements isn't guessed"
    def foo():
        warnings.warn("boo")
    with no_deprecated_call():
        foo()

this patch gets the new test form working as you might expect from above analysis:

--- a/package/MDAnalysis/core/universe.py
+++ b/package/MDAnalysis/core/universe.py
@@ -145,7 +145,7 @@ def _resolve_coordinates(filename, *coordinates, format=None,
             get_reader_for(filename, format=format)
         except (ValueError, TypeError):
             warnings.warn('No coordinate reader found for {}. Skipping '
-                            'this file.'.format(filename))
+                            'this file.'.format(filename), UserWarning)
         else:
             coordinates = (filename,) + coordinates
     return coordinates
diff --git a/package/MDAnalysis/topology/ITPParser.py b/package/MDAnalysis/topology/ITPParser.py
index d2f7b6da8..ff47f0728 100644
--- a/package/MDAnalysis/topology/ITPParser.py
+++ b/package/MDAnalysis/topology/ITPParser.py
@@ -607,7 +607,7 @@ class ITPParser(TopologyReaderBase):
             warnings.warn("Element information is missing, elements attribute "
                           "will not be populated. If needed these can be "
                           "guessed using universe.guess_TopologyAttrs("
-                          "to_guess=['elements']).")
+                          "to_guess=['elements']).", UserWarning)


warned = [warn.message.args[0] for warn in record]
assert "Element information is missing" in warned[0]
assert "No coordinate reader found" in warned[1]
Loading