openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
321 stars 93 forks source link

Is "!1..." a typo in the smirnoff99Frostt #247

Closed drmeister closed 2 years ago

drmeister commented 5 years ago

https://github.com/openforcefield/openforcefield/blob/master/openforcefield/data/forcefield/smirnoff99Frosst.offxml#L196

The first atom test seems a bit odd - it looks like it should read "[!#1:1]..." - otherwise it will miss other isotopes of hydrogen.

andrrizzi commented 5 years ago

Thanks for the report! This is probably a good question for @bannanc or @davidlmobley .

andrrizzi commented 5 years ago

It looks like smirnoff99Frosst.offxml file here is actually "[#1:1]", not [!1:1] so I think it is a typo and we should correct it soon. Thanks for catching it!

This is related to #117.

andrrizzi commented 5 years ago

Ah, no wait. For some reason the line number differs in the file we have here and in the smirnoff99Frosst repo. The parameter is the same over there as well (see here), but if I'm not mistaken, the two files are supposed to be identical so this requires further investigation.

bannanc commented 5 years ago

@andrrizzi I think the point of this issue is just that [!1:1] should be [!#1:1]. Since that parameter shouldn't be different in the file here or smirnoff99frosst the point of the issue is that we need to fix the typo...

I'm not sure what that has to do with issue #117

andrrizzi commented 5 years ago

False alarm. That was probably a browser cache issue. Sorry for the spam.

andrrizzi commented 5 years ago

I'm not sure what that has to do with issue #117

I thought the two files got out of sync, and installing the smirnoff99Frosst force field through the plugin architecture would avoid issues like this.

andrrizzi commented 5 years ago

Oh, the two files did get out of sync. In the new spec, the ImproperTorsions belong to their own tag: https://github.com/openforcefield/smirnoff99Frosst/blob/master/smirnoff99Frosst/smirnoff99Frosst.offxml#L139-L142, for example. Not sure if this is actually a problem, but we should probably make a new smirnoff99Frosst release.

jchodera commented 5 years ago

We'll definitely need a new release of the smirnoff99Frosst repo because that .offxml file uses the 0.1 version of the SMIRNOFF spec. I think this should probably be coordinated with enabling the entry point parameter directory plugin capability so that the toolkit will automatically recognize these files when the smirnoff99Frosst package is installed.

bannanc commented 5 years ago

Oh, the two files did get out of sync. In the new spec, the ImproperTorsions belong to their own tag: https://github.com/openforcefield/smirnoff99Frosst/blob/master/smirnoff99Frosst/smirnoff99Frosst.offxml#L139-L142, for example. Not sure if this is actually a problem, but we should probably make a new smirnoff99Frosst release.

We'll definitely need a new release of the smirnoff99Frosst repo because that .offxml file uses the 0.1 version of the SMIRNOFF spec. I think this should probably be coordinated with enabling the entry point parameter directory plugin capability so that the toolkit will automatically recognize these files when the smirnoff99Frosst package is installed.

These two things are both definitely true, we need a new SMIRNOFF99Frosst release with the new spec AND it would be great if those could just be pulled into the openforcefield toolkit when the force field is installed.

However, I think we want to keep those points separate from this issue which is just about a typo!

andrrizzi commented 5 years ago

However, I think we want to keep those points separate from this issue which is just about a typo!

Absolutely. I was just trying to figure out if this was a typo or a synchronization problem with the smirnoff99Frosst repo above. This is why I brought the problem up here, but I fully agree that the two issues are separate. Sorry if I made it confusing.

bannanc commented 5 years ago

Absolutely. I was just trying to figure out if this was a typo or a synchronization problem with the smirnoff99Frosst repo above. This is why I brought the problem up here, but I fully agree that the two issues are separate. Sorry if I made it confusing.

OK, that makes way more sense. Yes, if this was only happening in the new file that would be a BIG problem since all the SMIRKS should be the same.

I put an issue on the smirnoff99frosst issue tracker as well.

@drmeister Thank you for pointing this out, yes it is a typo and we will make sure to fix it!

drmeister commented 5 years ago

Ok - thanks. That breaks my offxml file reader - I guess that's what I get for writing a reader for v0.1 of anything. :-)

It's probably a reasonable time to tell you that I implemented a Smirnoff reader and parameter assignment code in Cando.

bannanc commented 5 years ago

@j-wags tagging this issue made me think. For our sakes do we want to keep issues like this in the openforcefield repo at all? I understand a user might write one here when they have a problem with smirnoff99Frosst, but I think the issue belongs on the SMIRNOFF99frosst issue tracker. I made an issue over there that points to this discussion already.

jchodera commented 5 years ago

Force field specific issues should ideally go in the force field repo, but until we require installation of a separate smirnoff99Frosst package and remove all force fields from the default unqualified namespace in this repo, we'll probably have to migrate a few issues over.

davidlmobley commented 5 years ago

Yes, agree it belongs there, @bannanc .

Can you make the change over there and deal with the changes here too so we can close, @bannanc ?

bannanc commented 5 years ago

Is the smirnoff99frosst repo up to date with the new spec? I didn't want there to be conflicts in files across the two repos. I'll put in a PR to both, in a second.

j-wags commented 5 years ago

Can we resolve the versioning (#249) issue before we fix this? I'm concerned that this fix will add three more identically-named files in our version histories, with two unique sets of contents.

mattwthompson commented 2 years ago

It's hard to follow the tea leaves on this one, but I think this is resolved given that we can load smirnoff99Frostt files from more recent releases.