choderalab / espaloma

Extensible Surrogate Potential of Ab initio Learned and Optimized by Message-passing Algorithm 🍹https://arxiv.org/abs/2010.01196
https://docs.espaloma.org/en/latest/
MIT License
203 stars 23 forks source link

Charge Method Question #141

Closed madilynpaul closed 11 months ago

madilynpaul commented 1 year ago

I just had a couple questions that I ran into when running Espaloma and reading through the docs.

First, it seems like the primary function of the openmm_system_from_graph is to create an openmm system that stores the information (forcefield parameters) obtained from espaloma. If this is the case, why is there an option to calculate the partial charges separately rather than using ones given by espaloma? Is there something about the partial charges given by espaloma that might give someone reason to re-calculate them?

Second, according to the doc strings, the default behavior should be to use the partial charges from espaloma, but the actual default behavior is to assign partial charges using the am1-bcc method. Is this a typo and/or mistake? Or is there a reason am1-bcc is chosen as the default instead of nn?

Thank you!

mikemhenry commented 1 year ago

Good catch!

@yuanqing-wang it looks like this doc string is wrong https://github.com/choderalab/espaloma/blob/master/espaloma/graphs/deploy.py#L45

I can make the PR, let me know what you want the default to be.

Re:

why is there an option to calculate the partial charges separately rather than using ones given by espaloma

That is so we can easily compare the charges espaloma assigns to other charge methods.

Let me know if you have any questions! :smile:

chrisjonesBSU commented 1 year ago

That makes sense. I think the main question we have is the choice of default. Re-calculating using another charge method can be quite slow for larger molecules, so perhaps the default behavior should be to use the values from espaloma instead of using another method.

chrisjonesBSU commented 1 year ago

I guess it doesn't really matter what the default is (as long as the doc strings match), I guess we were ultimately curious if there was a particular reason why am1bcc was set as a default instead of nn. We can explore both methods and decide which one works best for the molecules we're using.

mikemhenry commented 11 months ago

fixed with https://github.com/choderalab/espaloma/issues/141