Skip to content

Conversation

@spyke7
Copy link
Contributor

@spyke7 spyke7 commented Nov 21, 2025

Hi @PicoCentauri ,
See firstly I thought of using warnings, but saw that in the find_cls_members function, 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. is DeprecationWarning
  • And others are UserWarning
    So 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 DeprecationWarning and tried in both __init__.py and __main__.py. But it remained.

I found that in MDAnalysis/analysis/hbonds/hbond_autocorrel.py -
warnings.catch_warnings() context manager is used with warnings.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/

@spyke7
Copy link
Contributor Author

spyke7 commented Nov 21, 2025

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?

@PicoCentauri
Copy link
Collaborator

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:
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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)

@spyke7
Copy link
Contributor Author

spyke7 commented Nov 24, 2025

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.

Can you please guide me about the probable title or the description of the issue...
And I want to change this -
(hbond_autocorrel.py)

with warnings.catch_warnings():
    warnings.simplefilter("always", DeprecationWarning)
    wmsg = (
        "This module was moved to "
        "MDAnalysis.analysis.hydrogenbonds.hbond_autocorrel; "
        "hbonds.hbond_autocorrel will be removed in 3.0.0."
    )
    warnings.warn(wmsg, category=DeprecationWarning)

Here just changing the always in warnings.simplefilter

@PicoCentauri
Copy link
Collaborator

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.

Can you please guide me about the probable title or the description of the issue... And I want to change this - (hbond_autocorrel.py)

with warnings.catch_warnings():
    warnings.simplefilter("always", DeprecationWarning)
    wmsg = (
        "This module was moved to "
        "MDAnalysis.analysis.hydrogenbonds.hbond_autocorrel; "
        "hbonds.hbond_autocorrel will be removed in 3.0.0."
    )
    warnings.warn(wmsg, category=DeprecationWarning)

Here just changing the always in warnings.simplefilter

I would say something along the lines Don't always warn in hbond_autocorrel. In the description you can also link this PR.

@spyke7
Copy link
Contributor Author

spyke7 commented Nov 27, 2025

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.

@PicoCentauri
Copy link
Collaborator

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.

@spyke7
Copy link
Contributor Author

spyke7 commented Nov 28, 2025

Hi @PicoCentauri
I added the tests please check it and tell me if I need to add or edit anyother things
And inside mdanalysis I talked with the maintainers and proposed them for removing the always word but they suggested me to replace find_cls_members with an explicit list of classes, removing hbonds.hbond_autocorrel from the list.
Please check the issue there.

@PicoCentauri
Copy link
Collaborator

Skipping this class should be rather easy. There is already a list here:

skip_mods = [
"AnalysisFromFunction",
"HydrogenBondAnalysis",
"WaterBridgeAnalysis",
"Contacts",
"PersistenceLength",
"InterRDF_s",
]

we could just add the hbond code to it.

@spyke7
Copy link
Contributor Author

spyke7 commented Nov 28, 2025

I checked for class names and even tried to include HydrogenBondAutoCorrel inside the skip_mods but it didn't skip it.
I ran this as a test file -

from MDAnalysis.analysis import __all__
print([m for m in __all__ if 'hbond' in m.lower()])

and got ['hbonds']
So I skipped it from the module_list, by adding m!=hbonds

@PicoCentauri
Copy link
Collaborator

Okay, weird. But I think we had a similar issue before.

Cool that it is working. I will check the code next week.

Copy link
Collaborator

@PicoCentauri PicoCentauri left a 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.

@PicoCentauri PicoCentauri merged commit 22e8b6c into MDAnalysis:main Dec 2, 2025
11 checks passed
@spyke7
Copy link
Contributor Author

spyke7 commented Dec 2, 2025

Welcome @PicoCentauri
I would like to solve more issues and also implement the new features

@PicoCentauri PicoCentauri mentioned this pull request Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants