-
Notifications
You must be signed in to change notification settings - Fork 648
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 guess_bonds and bond guessing kwargs in Universe #4757
base: develop
Are you sure you want to change the base?
Conversation
Hello @lilyminium! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-25 04:43:42 UTC |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4757 +/- ##
===========================================
- Coverage 93.65% 93.62% -0.03%
===========================================
Files 175 187 +12
Lines 21565 22632 +1067
Branches 3023 3023
===========================================
+ Hits 20196 21190 +994
- Misses 925 998 +73
Partials 444 444 ☔ View full report in Codecov by Sentry. |
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 only have minor documentation update requests, otherwise looks good.
package/CHANGELOG
Outdated
* The `guess_bonds`, `vdwradii`, `fudge_factor`, and `lower_bound` kwargs | ||
are deprecated for Universe creation. (Issue #4756, PR #4757) |
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.
Perhaps state what should be used instead? Such as "use Universe.to_guess(...) instead".
package/MDAnalysis/core/universe.py
Outdated
"Bond guessing through the `guess_bonds` keyword is deprecated" | ||
" and will be removed in MDAnalysis 3.0. " | ||
"Instead, pass 'bonds', 'angles', and 'dihedrals' to " | ||
"the `to_guess` keyword in Universe for guessing these. " |
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.
add a
.. deprecated:: 2.8.0
Will be removed in 3.0.0. Pass pass into Context creation or Universe.guess_TopologyAttrs.
under each of the deprecated keyword args fudge_factor
, vdwradii
, lower_bound
. Link to Context
docs and guess_TopologyAttrs
.
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.
Be specific about where the kwargs should go, either Context or guess_TopologyAttrs.
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.
A few extra thoughts:
- The order of the attributes in
['bonds', 'angles', 'dihedrals']
is important because later attributes depend on the earlier ones. We may want to implement a function that automatically reorders the guessing sequence to ensure proper dependencies are respected?
2. The correct usage should beforce_guess=['bonds', 'angles', 'dihedrals']
instead ofto_guess
. Refer to Unclear Error When Using to_guess=['bonds'] with Existing Bonds in Topology #4759
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 think the connectivity topology attrs do have dependencies built in, happily -- there's an existing test that guesses angles without bonds (
mdanalysis/testsuite/MDAnalysisTests/guesser/test_default_guesser.py
Lines 167 to 173 in d2729f7
def test_guess_angles_with_no_bonds(): | |
"Test guessing angles for atoms with no bonds" | |
" information without adding bonds to universe " | |
u = mda.Universe(datafiles.two_water_gro) | |
u.guess_TopologyAttrs(to_guess=['angles']) | |
assert hasattr(u, 'angles') | |
assert not hasattr(u, 'bonds') |
vdwradii
, etc. keywords used to control bond guessing though.
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.
Added in #4761
@@ -407,6 +407,17 @@ def __init__(self, topology=None, *coordinates, all_coordinates=False, | |||
self._trajectory.add_transformations(*transformations) | |||
|
|||
if guess_bonds: | |||
warnings.warn( | |||
"Bond guessing through the `guess_bonds` keyword is deprecated" |
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.
Add a deprecation doc string under guess_bonds
with this information so that it shows up in the docs.
package/MDAnalysis/core/universe.py
Outdated
"Bond guessing through the `guess_bonds` keyword is deprecated" | ||
" and will be removed in MDAnalysis 3.0. " | ||
"Instead, pass 'bonds', 'angles', and 'dihedrals' to " | ||
"the `to_guess` keyword in Universe for guessing these. " |
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.
A few extra thoughts:
- The order of the attributes in
['bonds', 'angles', 'dihedrals']
is important because later attributes depend on the earlier ones. We may want to implement a function that automatically reorders the guessing sequence to ensure proper dependencies are respected?
2. The correct usage should beforce_guess=['bonds', 'angles', 'dihedrals']
instead ofto_guess
. Refer to Unclear Error When Using to_guess=['bonds'] with Existing Bonds in Topology #4759
Fixes #4756
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4757.org.readthedocs.build/en/4757/