Deprecate guess_bonds and bond guessing kwargs in Universe #4757
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. |
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.
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.
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.
Be specific about where the kwargs should go, either Context or guess_TopologyAttrs.
There was a problem hiding this comment.
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.
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
vdwradii, etc. keywords used to control bond guessing though.
| @@ -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.
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.
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/