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

Strange hbond analysis deprecation #1668

Closed
kain88-de opened this Issue Sep 14, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@kain88-de
Member

kain88-de commented Sep 14, 2017

Expected behaviour

Using the hbond analysis doesn't raise any warnings

Actual behaviour

Our own test code raises the about 150+ deprecation warnings for hbonds.

_get_bonded_hydrogens_list() is deprecated but called internally by the hbond analysis all the time.

"all hydrogens; detect_hydrogens='distance' is safer.",

I'm not sure how to resolve this since it tells the user to initiate the class differently then to call this function. The code was written by @orbeckst in 2011. So does this still apply?

Currently version of MDAnalysis:

(run python -c "import MDAnalysis as mda; print(mda.__version__)")
0.17.dev

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Sep 15, 2017

Member

Is it really called "all the time"?

_get_bonded_hydrogens_list() is deprecated but called internally by the hbond analysis all the time.

because the default is 'distance'. 'heuristic' can be chosen to the user if they know what they are doing.

To be honest, I am not sure if we need to keep the 'heuristic' version around. We could simply retire it. I don't remember any benchmarking results that compared the speed difference between the two but one could argue that we rather be always correct than fast but only sometimes correct.

Member

orbeckst commented Sep 15, 2017

Is it really called "all the time"?

_get_bonded_hydrogens_list() is deprecated but called internally by the hbond analysis all the time.

because the default is 'distance'. 'heuristic' can be chosen to the user if they know what they are doing.

To be honest, I am not sure if we need to keep the 'heuristic' version around. We could simply retire it. I don't remember any benchmarking results that compared the speed difference between the two but one could argue that we rather be always correct than fast but only sometimes correct.

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Sep 15, 2017

Member

OK I misunderstood the deprecation warning, because it refers to a function name and a setting chosen in init. Thanks for clearing that up. This means for one we should update the warning message to make it clearer to the user and us. Also since this method is deprecated for years now. When do we want to remove it? Do we still want to remove it?

Member

kain88-de commented Sep 15, 2017

OK I misunderstood the deprecation warning, because it refers to a function name and a setting chosen in init. Thanks for clearing that up. This means for one we should update the warning message to make it clearer to the user and us. Also since this method is deprecated for years now. When do we want to remove it? Do we still want to remove it?

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Sep 15, 2017

Member

I just checked git blame and apparently I added this warning in 2012... I don't think that we even had a clear idea how we would deprecate features then. DeprecationWarning seems the wrong classification.

However, now I suggest to deprecate it for 1.0 – no-one should be using it but it will users time to react. (We can then remove the detect_hydrogens kwarg and the whole machinery to choose an algorithm.)

Member

orbeckst commented Sep 15, 2017

I just checked git blame and apparently I added this warning in 2012... I don't think that we even had a clear idea how we would deprecate features then. DeprecationWarning seems the wrong classification.

However, now I suggest to deprecate it for 1.0 – no-one should be using it but it will users time to react. (We can then remove the detect_hydrogens kwarg and the whole machinery to choose an algorithm.)

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Sep 15, 2017

Member

So rewording of the error message and giving a clear deprecation time. That's good. Then I can also appropriately filter the Deprecation warnings in our tests.

Member

kain88-de commented Sep 15, 2017

So rewording of the error message and giving a clear deprecation time. That's good. Then I can also appropriately filter the Deprecation warnings in our tests.

@orbeckst orbeckst added this to the 0.17.0 milestone Nov 7, 2017

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Nov 7, 2017

Member

This should be deprecated in 0.17.0 – just in case 1.0.0 actually happens after 0.17.0 ;-)

Member

orbeckst commented Nov 7, 2017

This should be deprecated in 0.17.0 – just in case 1.0.0 actually happens after 0.17.0 ;-)

@kain88-de kain88-de referenced this issue Dec 8, 2017

Merged

Dep message #1729

2 of 2 tasks complete

@orbeckst orbeckst closed this in #1729 Dec 9, 2017

@orbeckst orbeckst referenced this issue Dec 11, 2017

Merged

Water bridge prototype #1722

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment