Fixes for issues and assorted cleanups to generators #391
Conversation
* Remove residue_atoms argument of generate_residue_template() methods: fixes openmm#379 * OpenMMSystemMixin.convert_system_to_ffxml improper_atom_ordering was silently ignored * Removal of unused kwargs that could cause misspelled parameters to be silently ignored * Removal of some unused local variables * DummySystemGenerator: was broken due to early return * Consolidate custom classproperty implementation * Docstrings: parameters out of order, errors, formatting
* Use common logic for checking user-specified partial charges sum to total formal charge, and raising a warning if so * GAFFTemplateGenerator no longer silently adjusts user-specified partial charges if they don't sum to the formal charge: fixes openmm#373
* Consolidate propagate_dynamics() implementations into TemplateGeneratorBaseCase * Remove charges_from_system(), charges_are_equal(), compute_energy(), and compare_energies() from TestGAFFTemplateGenerator since identical implementations are available in TemplateGeneratorBaseCase
* Ensure warnings are raised / not raised as appropriate (GAFF, SMIRNOFF) * SMIRNOFF should accept user charges not matching once we warn about it
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
+ Coverage 83.16% 84.14% +0.98%
==========================================
Files 5 5
Lines 802 776 -26
==========================================
- Hits 667 653 -14
+ Misses 135 123 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This looks great! No idea what |
|
Thanks for taking a look. Once the espaloma tests are working, I can try merging that into here to verify that they pass. |
|
Looks like espaloma tests are running and passing on Python 3.11 and 3.12. |
mattwthompson
left a comment
mattwthompson
left a comment
There was a problem hiding this comment.
I spent about 15 minutes going through this and found only style suggestions and things that could be major changes but would cause all tests1 to fail if they were genuine issues. I did not look closely at the entire diff, if you feel like that is needed I will find the time.
Nice work - I can tell you're looking into things with detail.
Footnotes
-
I'm putting my trust in CodeCov here ↩
@mattwthompson Thanks for taking a look. If you do have time to look through other parts, and if you do have enough familiarity with them to review them, that could be very helpful. If not, no worries at all! @ anybody... See my comments above on |
|
@epretti Is this good to squash and merge? |
4 participants
A few items from the issue list:
GAFFTemplateGenerator#373:GAFFTemplateGeneratornow no longer silently adjusts user-specified partial charges if they do not sum to a molecule's formal chargeGAFFTemplateGenerator.generate_residue_template(residue_atoms=xxx)#379 by removingresidue_atomsargument ofgenerate_residue_template()methods (as discussed)I found that some areas of the generator code could use some cleaning up. Some areas I would appreciate any input on:
DummySystemGeneratorfeature was broken in multiple ways; it's unclear that it ever worked. Now fixed with a very basic test case added. Is/was there any actual use for this?template_generator_kwargsas an init parameter. This is only used by espaloma (note: it's not actually a**kwargs, just a single keyword argument!), but the other generators previously took and ignored**kwargs, silently swallowing misspelled parameters.Some minor changes:
classpropertyimplementations into oneimproper_atom_orderingargument ofOpenMMSystemMixin.convert_system_to_ffxml()being silently ignoredopenff-toolkitversion that is able to be installed with espaloma 0.3.2Though espaloma testing isn't set up in CI, I've checked the espaloma tests locally.