OFF-EP 0004 #24
OFF-EP 0004 #24
Conversation
|
I think the problem is probably a bit more difficult than what is addressed in the OFF-EP. In general, the spec is not amenable to people being able to add new parameter handlers for experimentation mainly because of issues like this. Namely, if I want to add a custom bond handler but also use constraints, then I will need to add a custom constraint handler as well instead of using the default constraints handler as it will raise an exception if Similarly, one could say that the v-site handler could require both a vdW and an electrostatics handler as it stores parameters that require information from both. But then if I want to use a custom vdW handler, I'm now again required to either add an empty vdW handler, or implement a custom v-site handler. So while I agree in principle with this PR, I think the larger ramifications it would have are problematic. This is probably a discussion that needs to be had on a call with all stakeholders involved. |
There was a problem hiding this comment.
This is a well-motivated OFF-EP.
However, it is not clear to me that changing the definition of all existing parameter handler specs to define required and optional parameter handlers is the best way to handle this, especially if it breaks backward compatibility.
There are two other approaches to consider:
- We could have each parameter handler explicitly specify "required_handlers" and "optional_handlers" as attributes or tags, making it explicit in the offxml what the dependencies are, and giving control over whether a handler should be required or optional (e.g. as noted for
Constraints, theBondssection could be required or optional depending on the content of theConstraintssection). - Another possibility is to create a specific constraint handler / validator section that specifies the dependency graph of all handlers, and this could override the default (historical) behavior. This would enable backwards compatibility.
Are these approaches inferior to the solution presented here?
| @@ -0,0 +1,100 @@ | |||
| # OFF-EP 0004 — Handlers specify their dependence on other handlers | |||
There was a problem hiding this comment.
Suggest "Handlers should explicitly specify their dependence on other handlers"
|
|
||
| ## Abstract | ||
|
|
||
| This OFF-EP requires that handlers state which other handlers they depend on, if any. |
There was a problem hiding this comment.
Suggest "requires that handlers explicitly state"
|
|
||
| This appears to only be an issue when handlers **require** information from other handlers. There | ||
| are some cases in which one section **could** use information from another but does not **require** | ||
| it. For example, a `<Bonds>` section does not **require** to information from other sections, |
There was a problem hiding this comment.
"For example, a <Bonds> section does not require to information from other sections,"
This sentence contains grammatical issues. I'm not sure what is meant.
|
|
||
| This proposal adds requirements to the specification that, in some cases, may require changes to | ||
| existing SMIRNOFF implementations. These changes should be improvements in the sense that | ||
| relationships between handlers will be clarified, but some changes may be breaking in the sense that |
There was a problem hiding this comment.
Suggest "some changes may be breaking" -> "some changes may break backward-compatibility"
|
|
||
| For example, continuing with the case of TIP3P water from above, a SMIRNOFF implementation after | ||
| this proposal should raise an exception if there is a `<LibraryCharges>` section present but no | ||
| `<Electrostatics>` sectoin present, since the form depends on the latter. This raised exception |
Resolves #2