Fix bond deletion - Githubissues

MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.

https://mdanalysis.org

Other

1.32k stars 651 forks source link

Fix bond deletion #4763

Closed lilyminium closed 3 weeks ago

lilyminium commented 4 weeks ago

Fixes #4762

Changes made in this Pull Request:

~~Changes Topology creation and addition to only accept a single identical bond/angle/dihedral/etc instead of multiple~~ (see discussion in PR)
Changes bond/angle/etc deletion to delete all copies of an identical bond if that bond is passed in for deletion

PR Checklist

[x] Tests?
[ ] Docs?
[x] CHANGELOG updated?
[x] Issue raised/referenced?

Developers certificate of origin

[x] 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--4763.org.readthedocs.build/en/4763/

pep8speaks commented 4 weeks ago

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-10-26 04:28:34 UTC

codecov[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.15%. Comparing base (d7153b9) to head (c64627e). Report is 3 commits behind head on develop.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #4763 +/- ## =========================================== - Coverage 93.59% 93.15% -0.45% =========================================== Files 177 12 -165 Lines 21708 1066 -20642 Branches 3052 0 -3052 =========================================== - Hits 20318 993 -19325 + Misses 943 73 -870 + Partials 447 0 -447 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

IAlibay commented 4 weeks ago

@lilyminium this PR is marked as draft - is it meant to be up for review or is there more work that needs to be done?

IAlibay commented 3 weeks ago

Failures aren't happening in other PRs - it looks like something that has been specifically introduced here.

lilyminium commented 3 weeks ago

Hmm. Yes that would be the case that multi-bonds can't be sketchily represented by digging into Topology internals, which downstream people may well be doing.

I can think of two solutions to this in this PR:

put that information in the order attribute (which TopologyObjects should have) -- a bit of work, may have time later today to push it through but not guaranteed. Also, even more changes in current behaviour
This PR fixes the deletion issue twice over. It first only counts identical bonds once when creating /adding to the topology object. It secondly also deletes all identical bonds when we try to delete. If we take out the first and just ensure that deletion works, we should still fix the original issue. This is a lot less work

lilyminium commented 3 weeks ago

Pushed a commit that removes the "addition" fixes and preserves previous behaviour (should be easy to revert if discussion goes the other way)

lilyminium commented 3 weeks ago

I'm leaning towards the simpler fix of just ensuring that all identical bonds are deleted when deletion is caused, as it's less disruptive to current behaviour, and potentially fixing the addition of identical bonds in a separate issue/PR later, so I've updated the changelog too to reflect that -- should still be easy to revert if there's any disagreement

IAlibay commented 3 weeks ago

Going with just the bond deletion fix seems like a reasonable thing for this release.