Skip to content

Conversation

@lohedges
Copy link
Contributor

This PR closes #371 by ensuring that the output of Connectivity::findPath is reproducible and considering all 4 atom paths when adding missing dihedrals for when looping over 1-4 pairs. I have tested this via the SOMD1 compatibility layer in SOMD2, but can try to come up with a unit test here if desired.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have added a changelog entry to the changelog (we will add a link to this PR as part of the review): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

@lohedges
Copy link
Contributor Author

Oh, and just to add that I've checked that the legacy AmberParams tests still pass.

Copy link
Contributor

@chryswoods chryswoods left a comment

Choose a reason for hiding this comment

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

Looks good - I think the only thing to keep an eye on is if the cost of finding all paths becomes very high for molecules with lots of rings?

@lohedges
Copy link
Contributor Author

Yes, good point. I'll get users to test. Hopefully we can run it over a range of inputs during the somd2 tests to see how it performs.

@lohedges
Copy link
Contributor Author

Actually, it will make no difference since findPath implicitly calls findPaths anyway. The only additional overhead will be from sorting the paths, but this should be (hopefully) offset by the fact that we now have an early exit condition for the dihedral addition loop, i.e. we don't add the same dihedral multiple times. In any case, we will see how things go once we've run a few benchmarks.

@lohedges lohedges merged commit 1175ce2 into devel Oct 22, 2025
4 of 5 checks passed
@lohedges lohedges deleted the fix_371 branch October 22, 2025 12:02
lohedges added a commit that referenced this pull request Oct 22, 2025
lohedges added a commit that referenced this pull request Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Sire::MM::AmberParams::validateAndFix isn't reproducibile

3 participants