Allow bonds etc to be additively guessed when present #4761

lilyminium · 2024-10-25T05:32:16Z

Fixes #4759

Changes made in this Pull Request:

Fixes bond caching when adding/removing bonds with guess_TopologyAttr
bonds, etc in force_guess now delete existing bonds, etc and replace them with a new guess
bonds, etc in to_guess simply are applied additively on top of the existing bonds
Changes force-guessing bonds (as in the original guesser PR) to additively guess them, which was the spirit of the previous bond guessing (as shown by some failing PDB tests in commit 1fc4514)

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--4761.org.readthedocs.build/en/4761/

pep8speaks · 2024-10-25T05:32:21Z

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 461:80: E501 line too long (86 > 79 characters)
Line 1244:1: W293 blank line contains whitespace
Line 1674:80: E501 line too long (87 > 79 characters)

In the file testsuite/MDAnalysisTests/guesser/test_base.py:

Line 233:1: W293 blank line contains whitespace

Comment last updated at 2024-11-11 08:38:31 UTC

lilyminium · 2024-10-25T07:25:14Z

(I have a fix for this but can't get tests to work due to #4762; fix incoming in #4763.)

lilyminium · 2024-10-26T04:42:23Z

(Current status of PR: tests are failing due to #4762, assuming they pass after that's resolved, this should be ready for review.)

lilyminium · 2024-10-26T15:04:02Z

@yuxuanzhuang if you have time to look at this, it'd be great if you could see if this resolves the original issue in #4759!

codecov · 2024-10-26T15:48:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.64%. Comparing base (800b4b2) to head (839e7f3).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4761      +/-   ##
===========================================
+ Coverage    93.59%   93.64%   +0.04%     
===========================================
  Files          177      189      +12     
  Lines        21710    22808    +1098     
  Branches      3052     3055       +3     
===========================================
+ Hits         20320    21358    +1038     
- Misses         943     1003      +60     
  Partials       447      447

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

IAlibay

First half of a review - blocking over the NoDataError check, but also some suggestions for some code comments to help future devs / reviewers.

package/MDAnalysis/core/universe.py

IAlibay · 2024-10-27T19:57:15Z

package/MDAnalysis/core/universe.py

@@ -1640,23 +1649,32 @@ def guess_TopologyAttrs(
                    fg =  attr in force_guess
                    try:
                        values = guesser.guess_attr(attr, fg)
-                    except ValueError as e:
+                    except NoDataError as e:


Do we really want to do this? There are a couple of cases in guess_bonds where we can encounter a ValueError instead of a NoDataError.

e.g. https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/guesser/default_guesser.py#L430

Also what should happen when you encounter a KeyError? (i.e. the unrecognised topologyattr check you added)

Yup for this exact case -- I didn't want to change existing behaviour too much in case downstream packages were relying on it, and this is an existing test for vdwradii that wasn't introduced with guessers. The NoDataError is meant to catch cases where (e.g.) masses can't be guessed since there aren't types, not if the vdwradii is incomplete, so you can turn it off with error_if_missing. Same with the KeyError.

I'm sorry, I just don't follow what you mean here.

Oops. I hallucinated that you linked a test instead of a spot in the code, sorry about that. Basically IIRC there's two tests that test for this ValueError. One is definitely

mdanalysis/testsuite/MDAnalysisTests/core/test_universe.py

Lines 464 to 467 in e776f12

def test_universe_guess_bonds_no_vdwradii(self):

"""Make a Universe that has atoms with unknown vdwradii."""

with pytest.raises(ValueError):

mda.Universe(two_water_gro_nonames, guess_bonds=True)

and the other one I can't immediately recall. To preserve existing pre-guesser behaviour, we want the ValueError to pass through this check and get caught by those tests.

These passed in the initial guesser PR because that was pre #4754.

Yes, keeping the original behavior makes sense to me (and I confirm it is the original behavior).

The NoDataError is meant to catch cases where (e.g.) masses can't be guessed since there aren't types, not if the vdwradii is incomplete, so you can turn it off with error_if_missing. Same with the KeyError.

I'm not sure I understand what you mean by "same with the KeyError" - i.e. there is nothing here that catches the KeyError as far as I can tell.

For the sake of not stalling things more I'm going to go with it (i.e. the KeyError shouldn't happen under normal circumstances), but I would like to know what you meant here @lilyminium - it's unclear if you meant that the KeyError should behave like the NoDataError, in which case this except should include the KeyError too?

package/MDAnalysis/core/universe.py

testsuite/MDAnalysisTests/guesser/test_base.py

IAlibay · 2024-10-27T20:23:26Z

Thanks for the work here @lilyminium - overall this looks good, but I'm blocking on a bit more discussion for the NoDataError and the "duplicate old bonds" test.

orbeckst · 2024-10-28T01:06:37Z

I don't know the topology/guessers well enough to give informed input here, sorry.

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

yuxuanzhuang

After testing a few corner cases and comparing it with the previous behavior, I think it looks good to me!

yuxuanzhuang · 2024-11-01T19:01:59Z

package/MDAnalysis/core/universe.py

@@ -1640,23 +1649,32 @@ def guess_TopologyAttrs(
                    fg =  attr in force_guess
                    try:
                        values = guesser.guess_attr(attr, fg)
-                    except ValueError as e:
+                    except NoDataError as e:


Yes, keeping the original behavior makes sense to me (and I confirm it is the original behavior).

IAlibay

I have one follow-up question that I'm going to punt to later, and one formatting thing that I'll self merge. Otherwise lgtm.

testsuite/MDAnalysisTests/guesser/test_base.py

IAlibay · 2024-11-11T08:24:56Z

package/MDAnalysis/core/universe.py

@@ -1640,23 +1649,32 @@ def guess_TopologyAttrs(
                    fg =  attr in force_guess
                    try:
                        values = guesser.guess_attr(attr, fg)
-                    except ValueError as e:
+                    except NoDataError as e:


The NoDataError is meant to catch cases where (e.g.) masses can't be guessed since there aren't types, not if the vdwradii is incomplete, so you can turn it off with error_if_missing. Same with the KeyError.

I'm not sure I understand what you mean by "same with the KeyError" - i.e. there is nothing here that catches the KeyError as far as I can tell.

IAlibay · 2024-11-11T08:37:03Z

package/MDAnalysis/core/universe.py

@@ -1640,23 +1649,32 @@ def guess_TopologyAttrs(
                    fg =  attr in force_guess
                    try:
                        values = guesser.guess_attr(attr, fg)
-                    except ValueError as e:
+                    except NoDataError as e:


For the sake of not stalling things more I'm going to go with it (i.e. the KeyError shouldn't happen under normal circumstances), but I would like to know what you meant here @lilyminium - it's unclear if you meant that the KeyError should behave like the NoDataError, in which case this except should include the KeyError too?

add tests

6ee7c17

lilyminium added 4 commits October 25, 2024 16:33

special case connections

ada6967

pep8 tests

5ce43e0

add failing force_guess bond test

7db0f6a

add force guessing removal

f22a9a6

fix pep8

e21059c

lilyminium mentioned this pull request Oct 25, 2024

Unclear Error When Using to_guess=['bonds'] with Existing Bonds in Topology #4759

Closed

lilyminium added 2 commits October 25, 2024 18:30

cleanup

aed3d31

add more tests

e88af7a

lilyminium mentioned this pull request Oct 25, 2024

Deprecate guess_bonds and bond guessing kwargs in Universe #4757

Merged

5 tasks

update changelog

11a14c1

lilyminium marked this pull request as ready for review October 26, 2024 04:42

lilyminium and others added 12 commits October 26, 2024 15:43

Merge branch 'develop' into order-bond-guessing

523f06d

Merge branch 'develop' into order-bond-guessing

a27bebf

add tests

219ee09

add force guessing removal

1e6d9ab

add more tests

2918ae7

update tests after deletion fix

3e17007

fix problematic merge

892bc42

fix other merge problem

451d6a8

how did this rebase go so wrong

f044fa1

repep8

e17c57e

more merge fixes

1fc4514

change guessing bonds with force to additive

7305e60

lilyminium requested a review from yuxuanzhuang October 26, 2024 15:03

change valueerror to nodataerror

970371f

lilyminium requested review from orbeckst and IAlibay October 27, 2024 00:52

lilyminium added this to the Release 2.8.0 milestone Oct 27, 2024

add tests for errors

909ec37

IAlibay requested changes Oct 27, 2024

View reviewed changes

package/MDAnalysis/core/universe.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/guesser/test_base.py Show resolved Hide resolved

lilyminium and others added 8 commits October 29, 2024 06:28

Update package/MDAnalysis/core/universe.py

c190ca8

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update package/MDAnalysis/core/universe.py

f03a394

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update package/MDAnalysis/core/universe.py

80c68a6

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update package/MDAnalysis/core/universe.py

5ab97d4

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

add new test

38077d6

add guessing check

ab6418e

oopsie

26f91b5

update test

36b62e6

yuxuanzhuang approved these changes Nov 1, 2024

View reviewed changes

IAlibay approved these changes Nov 11, 2024

View reviewed changes

Update testsuite/MDAnalysisTests/guesser/test_base.py

839e7f3

IAlibay merged commit e6bc096 into MDAnalysis:develop Nov 11, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow bonds etc to be additively guessed when present #4761

Allow bonds etc to be additively guessed when present #4761

lilyminium commented Oct 25, 2024 •

edited by IAlibay

Loading

pep8speaks commented Oct 25, 2024 •

edited

Loading

lilyminium commented Oct 25, 2024

lilyminium commented Oct 26, 2024

lilyminium commented Oct 26, 2024

codecov bot commented Oct 26, 2024 •

edited

Loading

IAlibay left a comment

IAlibay Oct 27, 2024

IAlibay Oct 27, 2024

lilyminium Oct 28, 2024

IAlibay Oct 29, 2024

lilyminium Oct 29, 2024

yuxuanzhuang Nov 1, 2024

IAlibay Nov 11, 2024

IAlibay Nov 11, 2024

IAlibay commented Oct 27, 2024

orbeckst commented Oct 28, 2024

yuxuanzhuang left a comment

yuxuanzhuang Nov 1, 2024

IAlibay left a comment

IAlibay Nov 11, 2024

IAlibay Nov 11, 2024

	def test_universe_guess_bonds_no_vdwradii(self):
	"""Make a Universe that has atoms with unknown vdwradii."""
	with pytest.raises(ValueError):
	mda.Universe(two_water_gro_nonames, guess_bonds=True)

Allow bonds etc to be additively guessed when present #4761

Allow bonds etc to be additively guessed when present #4761

Conversation

lilyminium commented Oct 25, 2024 • edited by IAlibay Loading

PR Checklist

Developers certificate of origin

pep8speaks commented Oct 25, 2024 • edited Loading

Comment last updated at 2024-11-11 08:38:31 UTC

lilyminium commented Oct 25, 2024

lilyminium commented Oct 26, 2024

lilyminium commented Oct 26, 2024

codecov bot commented Oct 26, 2024 • edited Loading

Codecov Report

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Oct 27, 2024

orbeckst commented Oct 28, 2024

yuxuanzhuang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyminium commented Oct 25, 2024 •

edited by IAlibay

Loading

pep8speaks commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 26, 2024 •

edited

Loading