openmm
EspalomaTemplateGenerator default charges unclear
The EspalomaTemplateGenerator docstring states that the default charges are nn (those predicted by espaloma) however it seems llike they default to user charges and later then fall back to nn charges if the user charges are not present. I think the docstring just needs to be updated to make this clear or the behaviour should be updated to match what was intended?
Yes, the docstring doesn't seem to match the behavior. I don't really know what's the best here, but I'm willing to believe that using the charges from the molecule is probably a good enough default behavior. There are other places in the workflow (maybe in other projects) where we assume similar things.
At one point we had updated the behavior of the other template generators to use user charges as is if they are present, and otherwise assign charges per the preferred method for the force field (previously it was inconsistent).
I think what is inconsistent about EspalomaTemplateGenerator right now is that if template_generator_kwargs is None, you get from-molecule (which tries to read user charges first and falls back to nn with a warning otherwise), and if template_generator_kwargs is not None but charge_method is not provided within, you get nn and user charges are ignored. (I haven't tested this; it's just from reading the code.) We definitely want only one kind of "default" behavior, so I think these should at least be made consistent.
How about if we get from-molecule, we try to read user charges and fall back to nn with a warning if they are absent; if we get nn or some other Espaloma-supported method, we ignore any user charges; if we don't get a charge_method, the default behavior can be to read user charges and fall back to nn without a warning if they are absent? The documentation, of course, can be updated to reflect whatever behavior we decide on.
I'm open to suggestions for other behavior though.
I know this is kind of annoying, but we probably want to do a parametric test of the possible combinations as feasible, and confirm the default behavior as well.
Absolutely, we should have a test case that covers all of the cases and checks that the appropriate charges get assigned.
I should be able to fix this; I just want to be certain what the default behavior ought to be, since it seems inconsistent right now. What I proposed above makes sense to me and I think is most consistent with how the rest of the package works, but I don't actually make use of this class, so I want to make sure this is a good approach, and do something different if not.
How about if we get from-molecule, we try to read user charges and fall back to nn with a warning if they are absent; if we get nn or some other Espaloma-supported method, we ignore any user charges; if we don't get a charge_method, the default behavior can be to read user charges and fall back to nn without a warning if they are absent? The documentation, of course, can be updated to reflect whatever behavior we decide on.
Yes I think this makes the most sense and is more consistent with the other generators good idea!