Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Handlers for DampedExp6810, AmoebaMultipoleForce, AxilrodTeller potentials #56

Merged
merged 59 commits into from
Oct 23, 2023

Conversation

aehogan
Copy link
Collaborator

@aehogan aehogan commented Apr 16, 2023

Description

This is staging PR for in progress work on three new handlers, DampedExp6810 from https://doi.org/10.1021/acs.jctc.0c00837, Multipole from AmoebaMultipoleForce, and AxilrodTeller 3 body potentials.

Todos

  • Create handlers in handlers/nonbonded.py
  • Create collections in collections/nonbonded.py
  • Add tests in tests/handlers
  • Final checks

Questions

  • Can we get charges from AM1BCCHandler in a more elegant way?

Status

  • Ready to go

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2023

Codecov Report

Patch coverage: 91.16% and project coverage change: +8.54% 🎉

Comparison is base (3df2d30) 75.20% compared to head (9d16428) 83.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   75.20%   83.74%   +8.54%     
==========================================
  Files           4        4              
  Lines         242      523     +281     
==========================================
+ Hits          182      438     +256     
- Misses         60       85      +25     
Flag Coverage Δ
unittests 83.74% <91.16%> (+8.54%) ⬆️

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

Files Changed Coverage Δ
smirnoff_plugins/collections/nonbonded.py 91.36% <89.79%> (-4.33%) ⬇️
smirnoff_plugins/handlers/nonbonded.py 92.40% <100.00%> (+7.03%) ⬆️

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

@aehogan aehogan changed the title Initial handlers [WIP] Handlers for DampedExp6810, AmoebaMultipoleForce, AxilrodTeller potentials Apr 16, 2023
@mattwthompson
Copy link
Member

ToolkitAM1BCCHandler is in a strange place now that its main purpose (assigning partial charges) no longer lives in the toolkit. Ultimately the best way is just to access Molecule.assign_partial_charges. Make sure you use "am1bccelf10" if it's available, following the SMIRNOFF spec. You can see my implementation here (a bit obfuscated, but you can get the general idea): https://github.com/openforcefield/openff-interchange/blob/v0.3.0/openff/interchange/smirnoff/_nonbonded.py#L531-L549

@mattwthompson
Copy link
Member

@aehogan let me know when you'd like a review of this

…3 not needed when permutation mode is correctly set to SinglePermutation
@aehogan
Copy link
Collaborator Author

aehogan commented Jun 6, 2023

@mattwthompson if you could look at how I deal with unique molecules / force.addParticle that would be great, I would like to use virtual charge sites in the future.

for _ in range(topology.n_atoms):

for unique_mol_index, mol_map in topology.identical_molecule_groups.items():

@mattwthompson
Copy link
Member

@aehogan I think everything from my first pass is resolved. Are there any other specific issues you could use feedback on, and is there more work you plan to do here? No rush from my end, but when it's ready to go I can give the code another light pass. In that case I'd trust that these plugins produce the results you expect it to produce, and I'd mostly be looking for things like reliability and potentially problematic corner cases.

@aehogan
Copy link
Collaborator Author

aehogan commented Jun 13, 2023

@mattwthompson I have one more thing I want to add, checking for a custom bonded force that has a coloumb's law expression and zero'ing out those charges as well. OpenFF interchange creates such a force to handle scaled 1-4 charge interactions and so it would be a safety feature.

Besides that I have checked a bunch of things such as high temperature, high pressure noble gas density isotherms with all combinations of potentials as well as running >1000 openff-evaluator simulations and haven't encountered any problems so far.

@aehogan
Copy link
Collaborator Author

aehogan commented Oct 23, 2023

@mattwthompson I'm good to merge this whenever you get a chance to look over it again, right now the buckingham water example simulation is crashing but my tests are working on my end.

@mattwthompson
Copy link
Member

Great, I'll see if I can fix upstream

@mattwthompson
Copy link
Member

@aehogan could you please

  1. Merge upstream back in once more (:tm:)
  2. Add the following to .git-blame-ignore-revs:
# Versioneer auto-generated
94a0ae93bc06fa2e1cef137eec3f7fdefc0f9c01

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

I'm going to trust my past self, your report, and the automation (no warnings or non-typing errors in logs) here

@mattwthompson mattwthompson merged commit 69cc47f into main Oct 23, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants