General HTF improvments and speed ups! #1566
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1566 +/- ##
==========================================
- Coverage 95.31% 93.36% -1.95%
==========================================
Files 172 172
Lines 14482 14492 +10
==========================================
- Hits 13803 13530 -273
- Misses 679 962 +283
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| unit=unit.nanometer) | ||
| for idx in range(n_atoms_old): | ||
| hyb_idx = self._new_to_hybrid_map[idx] | ||
| hyb_idx = self._old_to_hybrid_map[idx] |
There was a problem hiding this comment.
Partially fixes #1562 by fixing the functions highlighted.
I could also just read...
There was a problem hiding this comment.
Yes, we definitely have this fix in feflow at https://github.com/OpenFreeEnergy/feflow/blob/75391ae50fc069e7d8a84a3c8fa6d501a3570747/feflow/utils/hybrid_topology.py#L2782
| # check we can extract the correct positions for the end states | ||
| old_positions = htf.old_positions(htf.hybrid_positions) | ||
| # check the shape and positions match the input | ||
| assert old_positions.shape == (12, 3) |
There was a problem hiding this comment.
you can assert positions against htf._old_positions (same for the new ones)
|
No API break detected ✅ |
|
How much effort would it be to backport these changes to feflow? |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
This looks great, but I'm blocking with the idea that we need to backport the fixes to FEFlow so that there's parity and both sets of tests pass.
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
not merged in feflow, but tests pass
Partially fixes #1562 by fixing the functions highlighted.
We also improve the build time of the HTF by reducing the number of times we iterate over the openmm topology, see figure for average timings.
Tasks:
Checklist
newsentryDevelopers certificate of origin