Conversation
|
pre-commit.ci autofix |
|
Oh this is very promising -- how stable are these timings? Do you think it would be worth making a test that ensures (for this test case) |
Locally, they are within a 3 second window so we could make a timing test, though I am not sure how common that is?We could also provide a timing script to be run whenever someone touches the HTF, which should not be often. Any preference @atravitz ? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1858 +/- ##
==========================================
- Coverage 94.95% 90.67% -4.29%
==========================================
Files 217 217
Lines 20768 20746 -22
==========================================
- Hits 19720 18811 -909
- Misses 1048 1935 +887
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
With bccc53c the timings are now
|
|
There are a few ways to track regressions, one cool way is to make sure the algo doesn't change the way it scales: https://pythonspeed.com/articles/big-o-tests/ but we don't really have things set up to make that easy (but it would be good to know how the HTF scales with system size, so it might be worth doing) |
I like the idea of a timing script for now. I want us to add performance benchmarking like this, but structure is TBD, so pop it in the |
|
No API break detected ✅ Griffe output |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Overall looks great, but there's a few things - especially key collisions, that probably should be further discussed / handled.
| index1_hybrid, index2_hybrid, | ||
| [r0_old, k_old, r0_new, k_new]) | ||
| # track that we've added this bond | ||
| hybrid_bonds[self._pair_key(index1_hybrid, index2_hybrid)] = True |
There was a problem hiding this comment.
Is there any reason for this to be a dictionary? Could keeping a set or list of pair key indices be suitable?
|
|
||
| return [] | ||
| index1, index2, r0, k = bond_force.getBondParameters(bond_index) | ||
| bond_lookup[HybridTopologyFactory._pair_key(index1, index2)] = (r0, k) |
There was a problem hiding this comment.
Could we ever be in a situation where there are multiple bonds acting on the same pair of indices? It would be odd, ,but I don't think there's anything in HarmonicBondForce that prevents that.
Given the likely low cost, I would probably suggest capturing that case with a quick "if pairkey in bond_lookup" check.
| angle_lookup = {} | ||
| for angle_index in range(angle_force.getNumAngles()): | ||
| index1, index2, index3, theta0, k = angle_force.getAngleParameters(angle_index) | ||
| angle_lookup[HybridTopologyFactory._triplet_key(index1, index2, index3)] = (theta0, k) |
There was a problem hiding this comment.
Same as above re: bonds with the same keys, should we guard against a case where you could have the same key but with a different angle term?
| hybrid_index_list[2], hybrid_force_parameters | ||
| ) | ||
| # track that we have added this angle for the hybrid system | ||
| hybrid_angles[self._triplet_key(*hybrid_index_list)] = True |
There was a problem hiding this comment.
Same as above - how about using a list or a set?
|
|
||
| auxiliary_custom_torsion_force = [] | ||
| old_custom_torsions_to_standard = [] | ||
| # use sets to keep membership checks quick as systems have many torsions |
There was a problem hiding this comment.
Wouldn't sets drop torsions with equal indices? My understanding is that this is more common / acceptable.
| hybrid_force_parameters = [ | ||
| torsion_parameters[4], torsion_parameters[5], | ||
| torsion_parameters[6], 0.0, 0.0, 0.0 | ||
| torsion_parameters[4], torsion_parameters[5].value_in_unit(unit.radian), |
There was a problem hiding this comment.
It's not immediately clear to me, why the value_in_unit change?
|
|
||
| # Check to see if this term is in the olds... | ||
| term = [hybrid_index_list[0], hybrid_index_list[1], | ||
| term = (hybrid_index_list[0], hybrid_index_list[1], |
There was a problem hiding this comment.
Exact quality on converted floats seems like it would increase the likelyhood of a miss here - it would be better if the comparisons was done to a certain float level instead, e.g. truncating the floats to 5 d.p. or something.
| # Then this terms has to go to standard and be deleted... | ||
| old_index = auxiliary_custom_torsion_force.index(term) | ||
| old_custom_torsions_to_standard.append(old_index) | ||
| old_custom_torsions_to_standard.add(term) |
There was a problem hiding this comment.
[nit] (I know it's a holdover from the previous code) maybe we should rename this variable - it's confusing
| if particle_index in self._atom_classes['unique_old_atoms']: | ||
| # Get the parameters in the old system | ||
| old_index = hybrid_to_old_map[particle_index] | ||
| [charge, sigma, epsilon] = old_system_nonbonded_force.getParticleParameters(old_index) |
There was a problem hiding this comment.
If we're looking for other performance improvements - you could probably cache this?
5 participants
Try to speed up the HTF creation, targeting the torsion force and nonbonded exceptions as they scale terribly with system size. Local testing on a small protein-ligand system shows good improvements
Main
This branch
We probably need the tests in #1856 to confirm that nothing is broken with these changes.
TODO
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin