Skip to content
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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 26, 2024
Merged

Deprecate guess_bonds and bond guessing kwargs in Universe #4757

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9914b98
deprecate bond guessing
lilyminium Oct 20, 2024
ed8f791
pep8
lilyminium Oct 20, 2024
311d43b
Merge branch 'develop' into deprecate-guess-bonds
orbeckst Oct 24, 2024
78056eb
update deprecation warnings and docs
lilyminium Oct 25, 2024
d351903
Merge branch 'develop' into deprecate-guess-bonds
lilyminium Oct 26, 2024
8465031
Update package/MDAnalysis/core/universe.py
orbeckst Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package/CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ Changes
numpy.testing.assert_allclose #4438)

Deprecations
* The `guess_bonds`, `vdwradii`, `fudge_factor`, and `lower_bound` kwargs
are deprecated for Universe creation. (Issue #4756, PR #4757)
lilyminium marked this conversation as resolved.
Show resolved Hide resolved
* Unknown masses are set to 0.0 for current version, this will be depracated
in version 3.0.0 and replaced by :class:`Masses`' no_value_label attribute(np.nan)
(PR #3753)
Expand Down
11 changes: 11 additions & 0 deletions package/MDAnalysis/core/universe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
orbeckst marked this conversation as resolved.
Show resolved Hide resolved
" and will be removed in MDAnalysis 3.0. "
"Instead, pass 'bonds', 'angles', and 'dihedrals' to "
"the `to_guess` keyword in Universe for guessing these. "
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few extra thoughts:

  1. 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 be force_guess=['bonds', 'angles', 'dihedrals'] instead of to_guess. Refer to Unclear Error When Using to_guess=['bonds'] with Existing Bonds in Topology #4759

Copy link
Member Author

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')
). I think that means we don't need to specify the dependencies in order. Hmm... I should add a test that guessing angles/dihedrals does respect the vdwradii, etc. keywords used to control bond guessing though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in #4761

"The kwargs `fudge_factor`, `vdwradii`, and `lower_bound` "
"are also deprecated and will be removed in MDAnalysis 3.0, "
"where they should be passed into Context creation "
"or guess_TopologyAttrs instead.",
DeprecationWarning
)
force_guess = list(force_guess) + ['bonds', 'angles', 'dihedrals']

self.guess_TopologyAttrs(
Expand Down
8 changes: 8 additions & 0 deletions testsuite/MDAnalysisTests/guesser/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,11 @@ def test_partial_guess_attr_with_unknown_no_value_label(self):
top = Topology(4, 1, 1, attrs=[names, types, ])
u = mda.Universe(top, to_guess=['types'])
assert_equal(u.atoms.types, ['', '', '', ''])


def test_Universe_guess_bonds_deprecated():
with pytest.warns(
DeprecationWarning,
match='`guess_bonds` keyword is deprecated'
):
u = mda.Universe(datafiles.PDB_full, guess_bonds=True)
Loading