Fix #284

[xiki-tempula]

No description provided.

[codecov]

Codecov Report

Merging #285 (ee67283) into master (98487f8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #285   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          26       26           
  Lines        1763     1765    +2     
  Branches      380      380           
=======================================
+ Hits         1740     1742    +2     
  Misses          3        3           
  Partials       20       20           

Impacted Files	Coverage Δ
src/alchemlyb/estimators/mbar_.py	98.36% <100.00%> (+0.05%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[orbeckst]

• (EDIT) Please add reST markup .. deprecated:: 1.0.1 etc to documentation
• please add test that check for deprecation warning
• ignore the deprecation warning in any tests with AutoMBAR so that it doesn't litter the output too much, you can use a pytest decorator like @pytest.mark.filtewarnings("ignore:"From version 2.0.0, this will be replaced by the default alchemlyb.estimators.MBAR.").

[xiki-tempula]

@orbeckst I have done

• Please add reST markup .. deprecated:: 1.0.1 etc to documentation
• please add test that check for deprecation warning

I'm a bit reluctant to do

• ignore the deprecation warning in any tests with AutoMBAR so that it doesn't litter the output too much, you can use a pytest decorator like @pytest.mark.filtewarnings("ignore:"From version 2.0.0, this will be replaced by the default alchemlyb.estimators.MBAR.").

The AutoMBAR is only directly used in test_fep_estimators, which is now wrapped in pytest.deprecated_call. In other parts of the code, AutoMBAR is only used once in test_workflow_ABFE so might be good to just leave it there.

[orbeckst]

The warning shows up multiple times

src/alchemlyb/tests/test_convergence.py: 2 warnings
src/alchemlyb/tests/test_fep_estimators.py: 12 warnings
src/alchemlyb/tests/test_visualisation.py: 1 warning
src/alchemlyb/tests/test_workflow_ABFE.py: 10 warnings
  /home/runner/work/alchemlyb/alchemlyb/src/alchemlyb/estimators/mbar_.py:216: DeprecationWarning: From version 2.0.0, this will be replaced by the default alchemlyb.estimators.MBAR.
    warn(

I'd prefer not any of our own warnings making it to the test output if we can do so. With our deprecations we know exactly what's going on. By removing the warnings, we cut down on the noise.

What's your rationale for keeping them visible?

[xiki-tempula]

@orbeckst
I do agree that if we keep a test that triggers an unnecessary warning for an extended period of time, it is not ideal.

However, for this particular warning, It is a bit difficult to pinpoint the tests that use AutoMBAR as they are operated via the default options, so I cannot search for them. Given that we are going to release 1.0.1 very soon and then straight to 2.0, where I will then need to remove them, I think it might not worth the time of trying to find these tests.

[orbeckst]

Can we disable them globally (eg pytest.ini) and only enable them when we want to test them?

[orbeckst]

good solution!