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.

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

add default None -> UserWarning #4747

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Oct 20, 2024

Fixes #4746

Changes made in this Pull Request:

  • This mimics the Python warnings library by setting category=None to UserWarning by default

We don't appear to test our utilities anywhere so I haven't added a formal test -- but running it with #4744 worked.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4747.org.readthedocs.build/en/4747/

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.41%. Comparing base (557f27d) to head (ff63933).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4747      +/-   ##
===========================================
+ Coverage    86.08%   86.41%   +0.33%     
===========================================
  Files          177      189      +12     
  Lines        21742    22808    +1066     
  Branches      3055     3055              
===========================================
+ Hits         18717    19710     +993     
- Misses        2593     2666      +73     
  Partials       432      432              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# https://docs.python.org/3/library/warnings.html#warnings.warn
# if category is None, the default UserWarning is used
if category is None:
category = UserWarning
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is a surprisingly simple fix, very nice. Confirming it also works in my hands.

I'll approve but not merge yet given the release cycle situation.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

It would be best if this was applied to something in our tests - i.e. some kind of testing by application.

@orbeckst
Copy link
Member

orbeckst commented Dec 4, 2024

@tylerjereddy can you keep an eye on this PR, please? If you don't have the bandwidth, please unassign yourself again. Thanks.

@tylerjereddy
Copy link
Member

@orbeckst @lilyminium testing locally, we could just follow through with our original intention here and the develop branch will fail this test without the patch that Lily is providing here. This fails on develop and passes in this branch:

--- a/testsuite/MDAnalysisTests/topology/test_itp.py
+++ b/testsuite/MDAnalysisTests/topology/test_itp.py
@@ -26,6 +26,7 @@ import MDAnalysis as mda
 import numpy as np
 from numpy.testing import assert_allclose, assert_equal
 
+from MDAnalysisTests.util import no_deprecated_call
 from MDAnalysisTests.topology.base import ParserBase
 from MDAnalysisTests.datafiles import (
     ITP,  # GROMACS itp
@@ -490,3 +491,5 @@ def test_missing_elements_no_attribute():
         u = mda.Universe(ITP_atomtypes)
     with pytest.raises(AttributeError):
         _ = u.atoms.elements
+    with no_deprecated_call():
+        mda.Universe(ITP_atomtypes)

That's what we meant to do all along anyway, and should match Irfan's request as well--actually using the functionality to test it.

@orbeckst
Copy link
Member

orbeckst commented Dec 4, 2024

Sounds good @tylerjereddy !

lilyminium and others added 3 commits December 4, 2024 15:06
* Add a regression test that serves the dual purpose of ensuring
that the `no_deprecated_call` decorator behaves properly when
the warning category is `None`, and also enforces our intended
warnings behavior for the element guessing changes in MDAnalysisgh-4744.
@tylerjereddy
Copy link
Member

I rebased and pushed in my suggested adjustment.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm!

@IAlibay
Copy link
Member

IAlibay commented Dec 4, 2024

Looks like failures are related to the rdkit bug, so this should still be good to go.

@RMeli
Copy link
Member

RMeli commented Dec 6, 2024

@IAlibay are you suggesting to merge despite the failures?

@IAlibay
Copy link
Member

IAlibay commented Dec 6, 2024

@IAlibay are you suggesting to merge despite the failures?

If folks don't mind then yes, we'll need to fix the rdkit failure at some point but that's probably not happening today.

@tylerjereddy tylerjereddy merged commit 4e903c7 into MDAnalysis:develop Dec 6, 2024
20 of 24 checks passed
@tylerjereddy
Copy link
Member

Ok, I squash-merged given the exact same CI failures are seen in other recent unrelated PRs.

Technically I pushed in a commit here so that's a bit like self-merging, but obviously the changes are very small, not really user-facing, and the approval was from another core dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAINT, TST: no_deprecated_call modernization
5 participants