Deprecate guess_bonds and bond guessing kwargs in Universe

[lilyminium]

Fixes #4756

Changes made in this Pull Request:

• Adds deprecation warning to bond keywords

PR Checklist

• Tests?
• Docs?
• CHANGELOG updated?
• Issue raised/referenced?

Developers certificate of origin

• I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

---

📚 Documentation preview 📚: https://mdanalysis--4757.org.readthedocs.build/en/4757/

[pep8speaks]

Hello @lilyminium! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/core/universe.py:

Line 296:1: W293 blank line contains whitespace
Line 453:80: E501 line too long (81 > 79 characters)

Comment last updated at 2024-10-26 09:43:41 UTC

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.57%. Comparing base (d7153b9) to head (8465031).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4757      +/-   ##
===========================================
- Coverage    93.59%   93.57%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21708    22775    +1067     
  Branches      3052     3052              
===========================================
+ Hits         20318    21312     +994     
- Misses         943     1016      +73     
  Partials       447      447              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[orbeckst]

I only have minor documentation update requests, otherwise looks good.

[orbeckst]

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.

[orbeckst]

Be specific about where the kwargs should go, either Context or guess_TopologyAttrs.

[yuxuanzhuang]

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

[lilyminium]

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.

[lilyminium]

Added in #4761

[yuxuanzhuang]

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

[lilyminium]

Thanks for the reviews @orbeckst and @yuxuanzhuang! Do the updates resolve your concerns?

[yuxuanzhuang]

lgtm!

[orbeckst]

lgtm, thx