Check user charges are not changed by espaloma

[jthorton]

Add a test to make sure user charges are respected by the espaloma small molecule force field

Checklist

• All new code is appropriately documented (user-facing code must have complete docstrings).
• Added a news entry, or the changes are not user-facing.
• Ran pre-commit by making a comment with pre-commit.ci autofix before requesting review.

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).

• GPU integration tests
• example notebook testing
• packaging tests: run this for any large feature PRs or PRs that add test data.

Developers certificate of origin

• I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

[jthorton]

pre-commit.ci autofix

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.50%. Comparing base (44a3f7d) to head (5fd4562).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1707      +/-   ##
==========================================
- Coverage   95.47%   92.50%   -2.97%     
==========================================
  Files         187      187              
  Lines       16231    16244      +13     
==========================================
- Hits        15496    15027     -469     
- Misses        735     1217     +482     

Flag	Coverage Δ
fast-tests	92.50% <100.00%> (?)
slow-tests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[IAlibay]

Am I correct in understanding this is happening because "from-molecule" is the default argument if you don't pass any template kwargs? https://github.com/openmm/openmmforcefields/blob/main/openmmforcefields/generators/template_generators.py#L1735

I might be looking in the wrong place, but it seems conflicting with the docstring: https://github.com/openmm/openmmforcefields/blob/1c969a638f72e8814fd29c5fd86f1af37517a783/openmmforcefields/generators/template_generators.py#L1657-L1658

Given that the defaults for when you pass in the kwargs is different from when you don't, I suspect it's actually a bug that is working in our favour.

It might be good to deal with this upstream (even if it means it might break behaviour here).

[IAlibay]

This is why we should slowly rename things to offunit and ommunit, but maybe not in this PR.

[github-actions]

No API break detected ✅

[jthorton]

Am I correct in understanding this is happening because "from-molecule" is the default argument if you don't pass any template kwargs?

Yes thats right, so the doc string seems to be incorrect, while its working in a our favour now if we ever pass some args to the generator it could break in future so this test should give us peace of mind for now. I'll open an issue upstream to see what needs to be fixed.

[jthorton]

@IAlibay its slightly more complicated than we thought, I found that the section which assigns the charges in the espaloma template generator which does another check for user charges and if they are not present changes the method back to nn charges.

[IAlibay]

@IAlibay its slightly more complicated than we thought, I found that the section which assigns the charges in the espaloma template generator which does another check for user charges and if they are not present changes the method back to nn charges.

Could you expand on the complication here? I might be misunderstanding, but that bit of the code seems to be doing what I would expect. I.e. if it's got no charges it's going to have to generate some, might as well generate them with espaloma.

[jthorton]

My thinking was that it adds another layer of confusion with regards to the docstring, it should should say by default checks for user charges and then if not defaults to espaloma.