Add NAGLCharges section

[mattwthompson]

Please only consider the OFF-EP-0011.md file when reviewing this PR. The modifications in smirnoff.md are just a copy of part of the EP (where it says "this part will be pasted in verbatim") and a little housekeeping to disambiguate charge method precedence (which is also covered in the EP text).

This will pave the way for the upcoming openff-2.3.0 release with graph charges.

Please use the GitHub review mechanisms (Approve/Request Changes) to vote. One convention we commonly use around reviews is that minor changes can be filed under an Approve (just list whether each proposed change is “blocking” - that is “this is approved only if this change is made” - or “not blocking” meaning “I think this would be a nice change, but I’d approve this being merged without it”).

As listed in the PR, the voting period will be open for two weeks (starting May 21 2025):

Acceptance criteria Unanimity (4 approving reviews) or partial support (2 approvals and 2 week period with no reviews requesting changes)

[j-wags]

Overall this is a good start. I think the big fork in the road is whether we specify exactly what's meant for the Model section and charge enumeration behavior, or if we can refer to an external spec/API and not get lost in the details, or if we have the initial version of this spec refer to the behavior of one or more openff-nagl versions and refer folks to the implementation there.

I hadn't had high hopes for the standardization reactions section, but it seems straightforward enough that I'd be OK to go with it pretty much as written.

[j-wags]

I'd propose that the charge hierarchy with this change become

1. Pre-specified charges (charge_from_molecules)
2. Library charges
3. GNN charges
4. ToolkitAM1BCC charges
5. ChargeIncrementModel charges

(Reading over the current spec, I realized ChargeIncrementModel and ToolkitAM1BCC aren't assigned relative priority, I've commented on #68 to that effect since that's a better scope for this information)

[j-wags]

I think this would interact with vsites in the same way as the current charge assignment methods. That is: "assigning base charges which vsites may later modify".

Our longer-term roadmap includes possibly having NAGL assign charges directly to vsites, but all I can think adding of for this is (paraphrasing) "in the future, GNNCharges may also be able to directly assign charges to virtual sites, but we'll need to work on the details of that before it goes into the spec". I recall that adding forward-looking statements into the spec has caused confusion before, so I'd be in favor of leaving this out.

[jameseastwood]

To be discussed in the SMIRNOFF committee April 1.

[davidlmobley]

I had a few minor comments other than those discussed synchronously:

• Does "atom_average_formal_charge" need units?
  • And it needs some kind of description in the EP; average over what?
• The "motivation and scope" is slightly confusing. e.g.
  • "Using an external tool to bypass..."; we haven't mentioned an external tool as such yet; what is meant here? Are we advocating that or trying to avoid it? Are we just saying we want to extend the spec to allow use of OpenFF NAGL which is a GNN?
  • second par "which is likely to be the case" -- is it meant that these will affect any force field using these charges, which will soon be used in OpenFF 2.x.0 releases such as perhaps 2.3.0?
  • I am not completely certain of the point of the "OpenFF has produced a reference implementation..." paragraph

[j-wags]

I've completed a first round of edits and next will schedule a meeting with @lilyminium to discuss:

• the sub-elements supported by the bond_features section
• whether it still makes sense to strictly define the data types now that we're not using the Model section
• whether atom_in_ring_of_size should be a single element with multiple ring sizes listed as an attribute

[j-wags]

Before I open this EP for final review, I'm going to start a PR(s) in the relevant OpenFF repos with an initial implementation of support for the NAGLCharges section to see if there are any technical deficiencies in this plan.

[j-wags]

Alright, I'd like to open this up for a vote!

[lilyminium]

What's the reasoning behind starting the versioning at v0.3 again?

Otherwise LGTM except for the concern below. Thanks so much for putting this together Jeff!!

My one concern is the sha/Zenodo DOI and how that could work with multiple NAGL versions. Using the sha really does restrict you to only one possible model/file. The way it's currently written, the model file name must also match strictly so could only match one possible data file in the Zenodo record. I think this is probably going to be fine and NAGL should be able to work with legacy files programmatically, but I might need to take a couple days to think about how we ship force field files that we ensure are generalizable across different software versions for fundamental dependencies such as pytorch, numpy, even python given the dependence on pickling.

[j-wags]

What's the reasoning behind starting the versioning at v0.3 again?

It's a weird but important thing that will keep coming up - might be good to even put this into the EP template #71 (comment)

My one concern is the sha/Zenodo DOI and how that could work with multiple NAGL versions. Using the sha really does restrict you to only one possible model/file. The way it's currently written, the model file name must also match strictly so could only match one possible data file in the Zenodo record. I think this is probably going to be fine and NAGL should be able to work with legacy files programmatically, but I might need to take a couple days to think about how we ship force field files that we ensure are generalization across different software versions for fundamental dependencies such as pytorch, numpy, even python given the dependence on pickling.

Good catch.

TL;DR: I'm ambivalent about the SHA and could drop it. I think the DOI is pretty important, but would approve the first version of this spec without it if it's objectionable. I hadn't considered changes in dependencies that could make the model unreadable but I think that can be handled a variety of ways.

Long version:

Thanks for the input, you're thinking about this from another angle and it's really helpful, please take your time!

My thinking was roughly that:

• this is the first spec section that refers to data outside the OFFXML file, but since we have the philosophy that the force fields encodes all the details needed to simulate the system, we should have a strong system for finding those details.
• including the SHA was my first thought for making sure the file name and expected contents matched up, but it's not a very good one and I'd be happy to drop it
• ultimately, we want to somehow specify a mapping like {model file name: a place where users can get that file}.
  • the files that this mapping will point to don't exist yet, so we can't put the mapping IN the spec
  • Since the spec is open, we can't "hard code" something like "if no file with the requested name is available, look in openff-nagl-models", since that makes us gatekeepers to other people making spec-compliant models.
  • However anyone can mint a zenodo DOI and attach a file. (note that it's NOT my intention to have this DOI get all the citations, just to use it as an OpenFF-agnostic global lookup table for model files)

That was all of my thinking on DOIs/SHAs.

I hadn't considered that we might need to make new releases of old models in response to pytorch or numpy changes. Without giving it a lot of thought, I think we'd be kinda OK for the NAGL software to someday say "this version of the NAGL software can't load that NAGL model, you may need to downgrade", or if there's a lot of demand, for us to refresh old FF releases with updated NAGL models (ex. if openff-3.1.0's NAGL model isn't supported by a modern stack but people really want to use it, we could release openff-3.1.1 where the only change is that we've updated the NAGL model file)

[lilyminium]

An alternate proposal is to use a model name instead of file, and to leave the SHA/DOI untangling to the libraries themselves. I think this is similar in philosophy to the current ToolkitAM1BCC provider that species a method name, and leaves the implementation up to the backend. Am I overlooking anything dangerous, insecure, or underspecified?

However, I've been thinking this over for a few days and I think the extreme likelihood (90+%) is that this is fine. Assuming pickling doesn't completely break, I believe the next highest risk would be pytorch typing. Outside of that we should be able to fairly easily recover the data and reformat it to requirements for our other dependencies. Finally, I believe that we have decided that we do want to move to a more general GNNCharges spec for the next model, so the NAGLCharges spec may not be used extensively.

I'll leave this open until Jeff is back from leave to see if this merits further discussion, but for now my vote is a provisional approval.

[davidlmobley]

I am in general good with this, but I DO really want the DOI to be pointed to, even if it creates some slight risk of ambiguity.

So I vote to approve.

[davidlmobley]

I vote to approve

[lilyminium]

@j-wags and I discussed after stand-up today. A basic summary of the discussion is:

• in the scenario where we need to make a patch release for infrastructure updates (e.g. 3.1.1 for 3.1.0), one reason that's a downside is we may have a number of force fields that use the same charge model and have to make many patch releases. That's not overly burdensome however; in this spec that change should be mostly just changing the DOI to point at a different version, and we could also consider having support windows for force fields similar to normal software packages. (In fact we'll want to consider this anyway if we want to investigate dropping ambertools).
• I continue to think the likelihood of something breaking dramatically is reasonably low, and I hope that moving to a more general GNNCharges spec with NAGL2 will make this a moot point.
• we also discussed the prospect of multi-version DOIs vs a single-versioned DOI, and chose to stick with the latter as it's simpler.