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

Use gasteiger instead of am1bcc for large mols #300

Closed
wants to merge 1 commit into from

Conversation

jaketanderson
Copy link

This small edit makes toolkit assign gasteiger partial charges instead of am1bcc if the molecule's atom count is >= 150, for speed purposes.

@mattwthompson
Copy link
Collaborator

Looks like this is only for the GAFF template generator, correct? I'm not familiar enough with GAFF to judge whether or not this is canonical, though I do understand why one would want this.

@jaketanderson
Copy link
Author

Yeah I think partial charge assignment via toolkit only happens for the GAFF generator. The supposition here is that as the molecule grows large, the computational cost of AM1BCC grows faster than does the rate of mistypings made using Gasteiger. This is just my intuition though...

@j-wags
Copy link
Contributor

j-wags commented Oct 10, 2023

In its current form I'd consider this unwanted behavior, since it will change the meaning of "GAFF" and many users would miss the warning and publish flawed studies. I'm not a maintainer, but as a community member I'd think this should only be triggered if the user sets a kwarg like use_gasteiger_for_large_molecules=True.

@mattwthompson
Copy link
Collaborator

In that case #296 and letting users to do this themselves would be a simpler fix

@jaketanderson
Copy link
Author

@j-wags that is a valid concern, I appreciate you saying that. If I'm the only person really facing this problem then I'm fine with editing my local openmmforcefields package. I don't want to add confusion for everyone else at the sake of my own convenience :)

@j-wags
Copy link
Contributor

j-wags commented Oct 10, 2023

@jaketanderson Thanks for understanding and working with the changes locally. And I agree that #296 would be a more graceful solution for this!

@mattwthompson
Copy link
Collaborator

In this case I think I have to be the bad guy and close this - hopefully the charge_from_molecules pathway can be enabled soon!

AM1-BCC for large molecules is a common pitfall for users, so if this is enabled in the future it would make a nice example. It shouldn't take too much code when supported: assign charges using the Molecule API and show how charge_from_molecules can be fed through the GAFF template generator to bypass AM1-BCC.

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