Handle some edge cases with preset charges #1070
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mattwthompson
force-pushed
the
preset-charges
branch
from
c205ffc
to
4099a1e
Compare
There was a problem hiding this comment.
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.
| 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 ChargeIncrementModels and the charge_increments 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.
| 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.
| * 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.
| 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.
| "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.
| 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.
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_moleculesargument from the toolkit. Resolves #1057 #1058 #1059Checklist
charge_from_moleculescontains any molecule without partial chargescharge_from_moleculescontains any duplicate molecules (defined by isomorphism, approximated by SMILES without hydrogens)charge_from_moleculeson virtual sites / charge increments can be unexpected. #1050from_smirnoffdocstringAdd FAQ entryI guess we don't have an FAQ here, maybe should checkopenff-docs