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

Jump to bottom

Allow bonds etc to be additively guessed when present #4761

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open

Allow bonds etc to be additively guessed when present #4761

wants to merge 23 commits into from
+197 −29

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Oct 25, 2024 •
edited by IAlibay
Loading

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

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

@pep8speaks
Copy link

pep8speaks commented Oct 25, 2024 •
edited
Loading

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

Line 221:1: W293 blank line contains whitespace

Comment last updated at 2024-10-27 01:41:54 UTC

@lilyminium
Copy link
Member Author

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

@lilyminium lilyminium mentioned this pull request Oct 25, 2024
Open
@lilyminium lilyminium mentioned this pull request Oct 25, 2024
Merged
5 tasks
@lilyminium
Copy link
Member Author

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

@lilyminium lilyminium marked this pull request as ready for review October 26, 2024 04:42
@lilyminium
Copy link
Member Author

@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 Codecov
Copy link

codecov bot commented Oct 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.64%. Comparing base (800b4b2) to head (909ec37).
Report is 1 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.

@lilyminium lilyminium added this to the Release 2.8.0 milestone Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuxuanzhuang yuxuanzhuang Awaiting requested review from yuxuanzhuang

@orbeckst orbeckst Awaiting requested review from orbeckst

@IAlibay IAlibay Awaiting requested review from IAlibay

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 2.8.0
Development

Successfully merging this pull request may close these issues.

Unclear Error When Using to_guess=['bonds'] with Existing Bonds in Topology
3 participants
@lilyminium @pep8speaks @IAlibay