-
Notifications
You must be signed in to change notification settings - Fork 637
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
deprecate hydrogen bond analysis in water bridge analysis to master #3111
Conversation
Hello @xiki-tempula! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-03-13 17:09:43 UTC |
Codecov Report
@@ Coverage Diff @@
## master #3111 +/- ##
==========================================
+ Coverage 90.96% 91.81% +0.84%
==========================================
Files 161 167 +6
Lines 22181 22699 +518
Branches 3198 3198
==========================================
+ Hits 20177 20841 +664
- Misses 1381 1774 +393
+ Partials 623 84 -539
Continue to review full report at Codecov.
|
Couple of things maintenance things:
Quick question @xiki-tempula, did you make any changes to the moved waterbridge analysis code (or it is just a standard move)? Github has done the really annoying thing of doing +1.5k -1.5k so it's pseudo-impossible to review anything from the diff. |
@IAlibay Nothing has changed in waterbridge analysis code. Just PEP8 changes. |
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.
Looks good (for 1.0.2). Thank you, @xiki-tempula !
@IAlibay you will merge this PR when 1.0.1 is out, right? |
Yup, I've placed it as a 1.0.2 milestone so we don't forget about it. |
@xiki-tempula if you can update against the current |
@IAlibay So I have merged the master and updated the changelog. |
Yes unfortunately it does. I think you should be able to use reload_module from six.moves instead? |
@IAlibay I think everything is fixed now. |
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.
Just some thoughts on py2 backward compatibility, and then one changelog addition.
of a water bridge existence. | ||
|
||
""" | ||
from __future__ import division |
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.
to ensure proper backwards compatibility we'll need six.move's range and zip too (worth having a look for other deprecated py2 things, but I think that's it?).
Probably worth adding absolute_import here too.
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.
I have added them but given that tests have all passed so I think should be fine.
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.
Cheers. Yeah, it's really just for the sake of "correctness".
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.
Thanks @xiki-tempula I'll merge once CI returns green.
…s#2913) * Move water bridge analysis from hbonds to hydrogenbonds (PR MDAnalysis#2913) * part of MDAnalysis#2739 * deprecate hydrogen bond analysis in water bridge in PR MDAnalysis#3111 * Update CHANGELOG Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
deprecate hydrogen bond analysis in water bridge analysis.
The hydrogen bond analysis is not used in the original water bridge analysis and I have cleared some reference to the hydrogen bond analysis in the doc sections.
Move water bridge analysis from hbonds to hydorgenbonds
Related to #2913, #2739 and #2746
PR Checklist