-
Notifications
You must be signed in to change notification settings - Fork 8
Removed warnings only for warnings with category : UserWarning #129
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
Conversation
|
I will update the CHANGELOG later for the docs to be passing. And should I generate a PR in Mdanalysis regarding this a probably changing hbonds a little? |
Yes, a PR in MDA that allows setting an own context filter should be good. Not allowing to disable warnings is a bit agains the zen of Python. |
| Flag to ignore warnings | ||
| """ | ||
| with warnings.catch_warnings(): | ||
| if not ignore_warnings: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hahaha this was easy.
Maybe we add a test for the ignore_warnings switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
should I add a test regarding this - about ignoring warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah basically, running this function once with ignore_warnings=True (checking that no warning will be emitted) and once with ignore_warnings=False (checking that a warning with pytest.warns is emitted)
Can you please guide me about the probable title or the description of the issue... Here just changing the |
I would say something along the lines |
|
Generated PR in mdanalysis. Only the fix in find_cls_memebers is enough for suppressing the warnings. When it will be accepted I can work on tests. |
|
Yes nice I saw it. I think one can already now work on a test. Probably the test should not depend on mda itself because they might remove all warnings at some point and this will lead to a failing test. |
|
Hi @PicoCentauri |
|
Skipping this class should be rather easy. There is already a list here: Lines 32 to 39 in 6333fdf
we could just add the hbond code to it. |
|
I checked for class names and even tried to include and got ['hbonds'] |
|
Okay, weird. But I think we had a similar issue before. Cool that it is working. I will check the code next week. |
PicoCentauri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Thanks again @spyke7. After merge I will do a release.
|
Welcome @PicoCentauri |
Hi @PicoCentauri ,
See firstly I thought of using
warnings, but saw that in thefind_cls_membersfunction, the logic of ignoring warnings was backward. So I fixed it just by removing not keyword and it removed warning of category : UserWarning.I have previously run the
warnings.simplefilter("ignore")from warnings, and got that the warning -This module was moved to MDAnalysis.analysis.hydrogenbonds.hbond_autocorrel; hbonds.hbond_autocorrel will be removed in 3.0.0.isDeprecationWarningSo even if I use filterwarnings and gave category of UserWarning, they got removed as well.
But the problem was the first one which is still there.
I have used to filterwarning and gave category
DeprecationWarningand tried in both__init__.pyand__main__.py. But it remained.I found that in
MDAnalysis/analysis/hbonds/hbond_autocorrel.py-warnings.catch_warnings()context manager is used withwarnings.simplefilter("always", DeprecationWarning)which always shows it even if I try to filterit.So in the moment I think either we fix the hbond_autocorrel.py or try to monkey patch (I don't know if it would work as well 🥲)
So please check the things and update me, I will try to help.
📚 Documentation preview 📚: https://mdacli--129.org.readthedocs.build/en/129/