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

started adding ether trappe-ua to forcefield #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

richardjgowers
Copy link
Contributor

I'm trying to add the parameters from here: https://pubs.acs.org/doi/abs/10.1021/jp049459w

couple questions

  • is this a good idea? or is the idea that people make their own xml files and store these locally?
  • the tests are confusing, they just seem to check that you can run atomtyping without it erroring, but doesn't actually check what happens in this process?

@mattwthompson
Copy link
Member

Thanks for the contribution!

is this a good idea? or is the idea that people make their own xml files and store these locally?

A good idea, and a little bit of both. We started this project with the idea that we'd build up huge XMLs of general force fields (OPLS-AA, GAFF, TraPPE, etc.) stored here but have since moved toward smaller, more manageable XMLs that cover just the scope of a particular system or project and stored elsewhere, like here. Since this is adding coverage to TraPPE it works fine here; other more specific force fields may be better hosted in other repos.

the tests are confusing, they just seem to check that you can run atomtyping without it erroring, but doesn't actually check what happens in this process?

Those tests check to see if it atomtypes, but also makes sure it generates the same atomtypes as the reference file, checking directly against the types you added to the mol2 file. The function atomtype in those tests isn't the main atomtyping engine, it's a testing function that types it and compares to the reference (maybe its name could be changed).

@richardjgowers
Copy link
Contributor Author

Ok I think this adds everything from that paper. Something I'm thinking about is maybe this could be split out into a new xml trappe-ethers.xml... the problem then being when I do acrylates next (into trappe-acrylates.xml) the "cross terms" between the two "expansions" won't be covered by either file. So maybe this monolithic file is better?

@mattwthompson
Copy link
Member

maybe this could be split out into a new xml trappe-ethers.xml... the problem then being when I do acrylates next (into trappe-acrylates.xml) the "cross terms" between the two "expansions" won't be covered by either file. So maybe this monolithic file is better?

In terms of what we want to ship with this package, a single monolithic file is definitely the target. There is virtually no added cost in using a large file (e.g. if you're only using 2 atomtypes, an XML with only those two atomtypes is the same cost as one with 200). For other systems (maybe if you're working on a project involving polymer that has some special chemistry that you want to toy around with the parameters of, create a custom atomtype, tinker with atomtyping rules, etc.) then the user can carry other smaller XML files for those purposes.

added atom types for
 - alkenes
 - ethers
 - acrylates
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.

2 participants