openforcefield / standards

A repository of the standards employed across the Open Force Field Consortium.
https://openforcefield.github.io/standards
MIT License
1 stars 3 forks source link

OFF-EP 0001 #21

Closed mattwthompson closed 2 years ago

mattwthompson commented 2 years ago

Resolves #5

mattwthompson commented 2 years ago

Can the FF apply a constraint that doesn't overlap a bond? I

I think so, at least per the spec. I can't say for sure how well-implemented this is in our software. The bond is only necessary for getting a constraint distance if not specified in the constraint parameter. Emphasis mine:

To constrain the separation between two atoms to their equilibrium bond length, it is critical that a record be specified for those atoms ...

Note that the two atoms must be bonded in the specified Topology for the equilibrium bond length to be used.

To specify the constraint distance, or constrain two atoms that are not directly bonded (such as the hydrogens in rigid water models), specify the distance attribute ...

j-wags commented 2 years ago

Thanks for the clarification, and sorry to waste your time to show me what was already there! 🤦

SimonBoothroyd commented 2 years ago

@mattwthompson because this is just a clarification of existing behaviour, and unless @davidlmobley or @j-wags disagrees, I'd recommend changing the status to accepted and merging this EOD Friday given that a majority has approved.

SimonBoothroyd commented 2 years ago

Please also update the mkdocs.yml and index.md files to include this OFF-EP

j-wags commented 2 years ago

Thanks for pushing on this, @mattwthompson!

I notice that we don't have a system here for the final stages of the EP process. Should the approvals have reset with the content changes? Should we wait for unanimous approval? Not sure about the former (I'm OK for approvals to be "sticky" in the interest of time, and I'll just dole them out more sparingly). The latter is probably "yes", since that's stated in the EP:

**Acceptance criteria:** Unanimity

So in the interest of process (which we can change in the future if we want!), I'm going to roll the merge back until we get a green check from everyone.

j-wags commented 2 years ago

Reverted in https://github.com/openforcefield/standards/pull/25

j-wags commented 2 years ago

Oh, I just read and saw @SimonBoothroyd's proposal from earlier:

@mattwthompson because this is just a clarification of existing behaviour, and unless @davidlmobley or @j-wags disagrees, I'd recommend changing the status to accepted and merging this EOD Friday given that a majority has approved.

Apologies that I missed this message and didn't object earlier.

I don't think the above process change is valid, since:

For sake of procedure, let's assume I would have removed my approval if the acceptance criteria had been changed from "Unanimity" to "Majority".

Sorry, I know that everyone is acting in good faith and we're all very busy. But this is an important process and we need to stick to it precisely. It contains the machinery to change itself, so if we want to change it to "majority" from here on, we could do an EP for that.

j-wags commented 2 years ago

@jchodera - Apologies, this was merged without your approval, and therefore it didn't reach the unanimity it requires. I've reverted the merge.

If this is in an acceptable state for you, please indicate your approval (a comment on this PR would be fine) and I'll re-merge it. If it's not, we may be able to resuscitate this PR, otherwise we can open another one to incorporate further changes.

jchodera commented 2 years ago

I'm not sure what the current status is here, but the changes to the SMIRNOFF spec are acceptable in the final version here.

j-wags commented 2 years ago

Great. Thanks, John! With your approval, it's unanimous. I'll re-merge it.