-
Notifications
You must be signed in to change notification settings - Fork 22
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
Handle some edge cases with preset charges #1070
base: develop
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
c205ffc
to
4099a1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great improvements I think. I think the documentation changes from #1048 could be merged with this so that the charge hierarchy is only documented once, which might improve findability - I think if there is similar documentation in two places, people sometimes read one place carefully, then later find the second place and think they're already familiar with it coz it looks the same.
My notes are mostly just adding more detail to what you've written, but this is great!
1. **Preset charges**: Look for molecule matches in the `charge_from_molecules` argument | ||
2. **Library charges**: Look for chemical environment matches in library charges | ||
3. **Charge increments**: Look for chemical environment matches in charge increments | ||
4. **AM1-BCC**: Try to run some variant of AM1-BCC (presumably this is were graph charges go) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. **Preset charges**: Look for molecule matches in the `charge_from_molecules` argument | |
2. **Library charges**: Look for chemical environment matches in library charges | |
3. **Charge increments**: Look for chemical environment matches in charge increments | |
4. **AM1-BCC**: Try to run some variant of AM1-BCC (presumably this is were graph charges go) | |
1. **Preset charges**: Look for molecule matches in the `charge_from_molecules` argument | |
2. **Library charges**: Look for chemical environment matches in the `<LibraryCharges>` section of the force field | |
3. **Charge increment models**: Look for chemical environment matches in the `<ChargeIncrementModel>` section of the force field | |
4. **AM1-BCC**: Try to run some variant of AM1-BCC (presumably this is were graph charges go) as described by the `<ToolkitAM1BCC>` section of the force field |
The spec uses the same language for ChargeIncrementModel
s and the charge_increment
s that are applied to move charges to virtual sites, so I think it's important to keep this distinction very clear. I also think it's clearer to specify the section of the force field that is being applied, instead of just repeating the name.
|
||
### Preset charges | ||
|
||
The following restrictions are in place when using preset charges: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following restrictions are in place when using preset charges: | |
The charges specified by the force field can be overridden by providing molecules with partial charges to the `charge_from_molecules` argument. This may be used to make use of alternate implementations of the appropriate charge generation method, or to provide different charges to the force field. Charges provided via `charge_from_molecules` are called "preset charges" because they are pre-set by the user, rather than computed by the force field. The following restrictions are in place when using preset charges: |
Just think this could use a little extra context.
* All molecules in the the `charge_from_molecules` list must be non-isomorphic with each other. | ||
* All molecules in the the `charge_from_molecules` list must have partial charges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* All molecules in the the `charge_from_molecules` list must be non-isomorphic with each other. | |
* All molecules in the the `charge_from_molecules` list must have partial charges. | |
* All molecules in the the `charge_from_molecules` list must be non-isomorphic with each other. | |
* All molecules in the the `charge_from_molecules` list must have partial charges. | |
* All copies of a molecule in the topology will be parametrized with the charges from an isomorphic molecule from the `charge_from_molecules` list. |
I think this one-to-many relationship is worth making explicit.
If specified, partial charges will be taken from the given molecules | ||
instead of being determined by the force field. | ||
instead of being determined by the force field. All molecules in this list | ||
must have partial charges assigned and must not be isomorphic with any other | ||
molecules in the list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If specified, partial charges will be taken from the given molecules | |
instead of being determined by the force field. | |
instead of being determined by the force field. All molecules in this list | |
must have partial charges assigned and must not be isomorphic with any other | |
molecules in the list. | |
If specified, partial charges for any molecules isomorphic to those | |
given will be taken from the given molecules' `partial_charges` | |
attribute instead of being determined by the force field. All | |
molecules in this list must have partial charges assigned and must | |
not be isomorphic with any other molecules in the list. |
I think this might help clear up the misconception that I sometimes see hinted at that this only affects the same molecule in the topology - like if I have four copies of a molecule, and pass literally one of those objects to charge_from_molecules
, then only that object will get those charges.
"Preset charges were provided (via `charge_from_molecules`) alongside a force field that includes " | ||
"virtual site parameters. Note that virtual sites will be applied charges from the force field and " | ||
"cannot be given preset charges.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Preset charges were provided (via `charge_from_molecules`) alongside a force field that includes " | |
"virtual site parameters. Note that virtual sites will be applied charges from the force field and " | |
"cannot be given preset charges.", | |
"Preset charges were provided (via `charge_from_molecules`) alongside a force field that includes " | |
"virtual site parameters. Note that virtual sites will be applied charges from the force field and " | |
"cannot be given preset charges. Virtual sites may also affect the charges of their orientation " | |
"atoms, even if those atoms are given preset charges.", |
Do you think adding something like this to the warning will be useful for people? It seems like the sort of thing that would surprise me but maybe it's just noise.
if molecules_with_preset_charges is None: | ||
return None | ||
|
||
molecule_set = {molecule.to_smiles() for molecule in molecules_with_preset_charges} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
molecule_set = {molecule.to_smiles() for molecule in molecules_with_preset_charges} | |
molecule_set = {molecule for molecule in molecules_with_preset_charges} |
Molecule.__eq__()
exists, so you should be able to make a set out of molecules directly. It definitely seems to work for me. Is matching SMILES faster for large topologies or something?
topology = Topology.from_molecules( | ||
[ | ||
Molecule.from_smiles("C"), | ||
Molecule.from_smiles("CCO"), | ||
], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this topology include a molecule with partial charges, to ensure that it's testing if there are any missing charges?
Description
This PR better handles user inputs around the
charge_from_molecules
argument from the toolkit. Resolves #1057 #1058 #1059Checklist
charge_from_molecules
contains any molecule without partial chargescharge_from_molecules
contains any duplicate molecules (defined by isomorphism, approximated by SMILES without hydrogens)charge_from_molecules
on virtual sites / charge increments can be unexpected. #1050from_smirnoff
docstringAdd FAQ entryI guess we don't have an FAQ here, maybe should checkopenff-docs