Discussion about explicit units for each parameter in SMIRNOFF version 0.3

[yudongqiu]

A while ago a discussion about API change leads to the adding of physical units explicitly for each parameter. #291

This leads to a new feature where parameters belongs to the same force field type can have different units, i.e., this is now supported by the openforcefield toolkit:

<Bond smirks="[#6X4:1]-[#6X4:2]" length="1.526 * angstrom" k="620.0 * angstrom**-2 * mole**-1 * kilocalorie" id="b1"></Bond>
<Bond smirks="[#6X4:1]-[#6X3:2]" length="0.151 * nanometer" k="265265.6 * nanometer**-2 * mole**-1 * kilojoule" id="b2"></Bond>

After discussion with @leeping, I think this extra flexibility of allowing multiple units for the same type of force constants is a bit unnecessary, and creates additional complications for parsing and converting the file. After a discussion with @j-wags, we think it's worth opening this up for discussion, so we can reach a consensus. Here, we provide some candidates for alternative formats for discussion and vote. The candidates are:

• (a) <Bond smirks="[#6X4:1]-[#6X4:2]" length="1.526 * angstrom" .. /> This is currently used in SMIRNOFF v0.3

• (b) <Bond smirks="[#6X4:1]-[#6X4:2]" length="1.526" length_unit="angstrom" .. /> This provides almost the same flexibility as (a) but allows easier parsing the xml file.

• (c) <Bonds length_unit="angstrom" .. /> The unit is defined in the parent element together with force type. This was used in SMIRNOFF v0.2

• (d) <Bond smirks="[#6X4:1]-[#6X4:2]" length="1.526" .. /> The default unit is fixed to be consistent with the default units in OpenMM.

A few points that may be worth considering:

• @j-wags mentioned options (a), (b), and (d) support the direct serialization of each term in the forcefield, but (c) does not.
• [option (d)] OpenMM and other existing simulation packages all define a single default unit that they use for their internal data. Experts in the field are most accustomed to this model. The responsibility to provide "correct" parameter values w.r.t. unit conversion is therefore on the force field writer, instead of the toolkit.
• [option (a)] The version 0.3 spec's units-with-values parameter format requires expensive parsing of each entry, which is computationally inefficient, and adds up to a significant fraction of ForceBalance's current runtime.
• New users may not be familiar with the simtk.unit package, and it is hard to find documentation of which units are supported.
• ForceBalance parses the offxml file in a similar way to the OpenMM xml file, and currently supports options (b), (c) and (d). It creates extra complexity if different explicit units are used, because the user needs to define the "prior width", indicating the magnitude of parameter change during fitting. The "prior width" are expected to be defined for a type of parameter that uses the same unit.

[j-wags]

So, I wrote the above along with Yudong. I'm hoping that this thread can help us reach an informed consensus on how to handle units in the future, since we made the choice to switch to solution (a) without including all project stakeholders in the discussion (and basically dropping the consequences in their lap). I'm also open to input from developers outside of OFF itself (every once in a while, I hear about outside folks interacting with the SMIRNOFF format, so please chime in here if these spec changes affect you!). But basically, I see @yudongqiu and @leeping as being much more expert than myself in this field, so this issue is worth revisiting.

I remain partial to option (a). My major reasoning is that, for all the pain a spec change now will cause, a spec change in the future will cause much more, as outside projects will become more dependent on our formats and behavior.

In order to get us through the current parameterization sprint, I can make a "spec downgrader", which will enable us to turn SMIRNOFF 0.3 spec files into 0.2 or 0.1 spec files, which can be read by ForceBalance. This should get us through September, and we can schedule time afterwards to properly integrate the OFF toolkit into ForceBalance.

So, I'd urge us to think about this question independent of the current time crunch. The biggest downsides of (a) are technical issues, which we can monkey patch around right now, and implement a robust solution as soon as there's time. The upsides of (a) are a tremendous amount of flexibility, and the most unambiguous possible representation of the values in our force field.

[jchodera]

The only articulated concerns seem to be

1. @yudongqui thinks this is "somewhat unnecessary"
2. It "creates additional complications for parsing and converting the file"

Please let me know if I missed a concern!

Item (1) is subjective, and easily countered by

• the compelling need to ensure the data model could be rendered in multiple serialization formats
• Real instances in which the old design led to cognitive errors and numbers were entered with the wrong units because the units were not tightly bound with the quantity they modify

I'd like to understand item (2). In this project, the only way you should be modifying SMIRNOFF files is via the OFF toolkit Python object model. I understand a quick and dirty implementation that modifies XML files directly was made, but this was made consciously recognizing it would be immediately replaced with the Python toolkit object model as soon as it became available.

Can you elaborate on issue (2)?

[yudongqiu]

@jchodera I agree the first point is subjective, but I really find it hard to understand the convenience of "having a mixture of units for same type of parameters in the file". While mistakes can be made when units are not explicitly specified for each parameter, I think it would be even easier to make mistakes when someone decide to mix more units into the file instead of keeping it consistent with the existing ones.

For the other quote, "the only way you should be modifying SMIRNOFF files is via the OFF toolkit Python object model", I agree it will be ideal if it's true. However if it's true then the point "Real instances in which the old design led to cognitive errors and numbers were entered with the wrong units because the units were not tightly bound with the quantity they modify" should not happen, because the API design does not depend on the file content, and we can specify units in the API.

In the case of ForceBalance, I'm afraid that substantial efforts will be needed to enforce this, because the current internal representation of the force field objective model is embedded very deeply into almost all components. On the other hand, making small tweaks to improve compatibility require much less work. I was under the impression that the second way is preferred in short term, but if I'm wrong I think this project need to start very soon because it needs to be done correctly before we can try any fitting.

[yudongqiu]

To illustrate on the performance concern @j-wags mentioned, in the fitting against ab initio data, such as torsion energies and optimized geometries, the ForceField and openmm system creation cost 60%-70% of the time, because we make small modifications to each value many times.

When working with offxml version 0.2, I found the wall time of evaluating one fitting target become 2x, when using the new OFF toolkit version 0.4.1 compared to the previous OFF toolkit 0.3. I think this might become a major bottleneck when we later have more ab initio data to fit against, and any speed improvement in the parsing of the offxml file would be very helpful in this case.

[jchodera]

However if it's true then the point "Real instances in which the old design led to cognitive errors and numbers were entered with the wrong units because the units were not tightly bound with the quantity they modify" should not happen, because the API design does not depend on the file content, and we can specify units in the API.

Agreed. Humans should not be editing the file, but that will happen no matter how hard we try. However, we do have full control over how our effort programmatically accesses the force field, and can enforce that.

I'm not sure I understand either this point:

In the case of ForceBalance, I'm afraid that substantial efforts will be needed to enforce this, because the current internal representation of the force field objective model is embedded very deeply into almost all components.

Could you elaborate on this? The rest of your notes seem to suggest you want to circumvent the toolkit entirely, and reimplement parts of the SMIRNOFF spec and its energy functions with specialized "tweaks" to existing ForceBalance code.

If the toolkit is slow for you, we should profile it to identify bottlenecks and speed up the slow parts. Otherwise, we run the risk of somehow having to validate two entirely different code paths for parsing SMIRNOFF or else we risk parameterizing one force field and applying the parameters in a different way via the toolkit.

[yudongqiu]

Could you elaborate on this? The rest of your notes seem to suggest you want to circumvent the toolkit entirely, and reimplement parts of the SMIRNOFF spec and its energy functions with specialized "tweaks" to existing ForceBalance code.

The current implementation uses the OFF toolkit to create OpenMM systems, and then evaluate energies and forces using OpenMM API. I think this part is consistent with the preferred implementation. The main difference is that currently ForceBalance uses its own forcefield objective model, which supports very flexible relations between parameters and many MD engines. Modifications of the forcefield parameters are done by writing out new files and use the MD engines to read the files. My idea is if we want to completely replace the current way of interacting with forcefield files by only allowing editing through the OFF toolkit, many components will need to be rewritten, and it's hard to predict how long it takes to validate the new implementation.

If the toolkit is slow for you, we should profile it to identify bottlenecks and speed up the slow parts. Otherwise, we run the risk of somehow having to validate two entirely different code paths for parsing SMIRNOFF or else we risk parameterizing one force field and applying the parameters in a different way via the toolkit.

I think the newly introduced feature that supports explicit units for each parameter is having a performance impact. We can try to minimize that but it will be hard to completely remove the impact because it's an additional dimension of flexibility that needs to be supported.

[davidlmobley]

@yudongqiu and @j-wags -- I think I'm slightly confused about part of the concern. The way this is framed:

This leads to a new feature where parameters belongs to the same force field type can have different units After discussion with @leeping, I think this extra flexibility of allowing multiple units for the same type of force constants is a bit unnecessary, and creates additional complications for parsing and converting the file

It sounds like Yudong and @leeping 's biggest concern is allowing multiple units for the same type of force constants. But... the proposed fixes aren't necessarily aimed at this, e.g., option (b) doesn't address it at all. Only options (c) and (d) address it, but both of these are (to me) unacceptable for two main reasons:

i) As John notes, we've entered things in the wrong units before. I personally have done this. Having the units on the lines will help avoid these problems. ii) Serialization -- my understanding was that options (a) and (b) (well, and (d) are much better for this. And since neither (a) nor (b) address your real concern as I understand it (and (a) is "status quo") I don't see any reason to change.

Just throwing it out there... but would it be easier for you guys, @yudongqiu and @leeping , if the units had to be CONSISTENT within any section, though they remained attached to individual parameters? And would this be hard from an implementation point of view, @j-wags ?

I think speed is a separate issue:

When working with offxml version 0.2, I found the wall time of evaluating one fitting target become 2x, when using the new OFF toolkit version 0.4.1 compared to the previous OFF toolkit 0.3. I think this might become a major bottleneck when we later have more ab initio data to fit against, and any speed improvement in the parsing of the offxml file would be very helpful in this case.

For example, we ought to be able to get dramatic speedups in assigning parameters to systems by simply implementing https://github.com/openforcefield/openforcefield/issues/51 and looking for SMIRKS matches in reverse order.

I think the newly introduced feature that supports explicit units for each parameter is having a performance impact.

We've always been committed to explicit units in the file. It's that we've moved the units from the categories (e.g. "bonds") to being attached to the individual parameters for more logical consistency and better serialization (a and b). Option (d) has always been out.

Basically, it sounds like (c) or (d) would be better for you because it's easier to get ForceBalance to work with the XML if you don't have to check the units of each individual parameter. But (d) has never been a possibility (and over my career I've spent far too long looking at docs relating to choices of units in different simulation packages to even consider it), and (c) hurts serialization, so as I see it we're going to have to go with (a) or (b).

[yudongqiu]

@davidlmobley Thanks for sharing your thoughts on the options.

Option (d) has always been out.

Sure as you said we can remove this to simplify the discussion.

It sounds like Yudong and @leeping 's biggest concern is allowing multiple units for the same type of force constants.

I agree that the biggest concern is allowing multiple units for the same type of force constants. Option (c) will enforce that so it's better than (a) and (b) in my mind, but the serialization problem seems to be very hard to solve technically, so I guess we need to remove option (c).

Among option (a) and (b), option (b) is directly supported by ForceBalance, and option (a) can be supported by small tweaks in the code. The real concern is that the parameter of the same type can have values vary by many orders of magnitude, when different units are used. This could cause trouble, and I think it only brings a small benefit that it could save a few seconds from unit conversion for people who are already familiar with the simtk.unit package that are manually writing new terms into the file. (If people use the OFF toolkit API to modify the file, then unit conversion can be done before exporting the offxml file.)

Just throwing it out there... but would it be easier for you guys, @yudongqiu and @leeping , if the units had to be CONSISTENT within any section, though they remained attached to individual parameters?

Exactly, if the extra flexibility of "allowing multiple units used for parameters of the same type" is removed, then both (a) and (b) are good with me. I slightly favor option (b) as it's easier to parse in programs.

For example, we ought to be able to get dramatic speedups in assigning parameters to systems by simply implementing #51 and looking for SMIRKS matches in reverse order.

I have already implemented hacks to cache the parameter assignments, so I guess this wouldn't improve my timings by much. Before the implementation of the caches, it takes 10x longer and a single fitting will take months. The current issue is the ForceField object creation using toolkit takes ~5x longer than before for offxml version 0.2. (For offxml version 0.3 it might be better but I don't have benchmarks).

[yudongqiu]

An update: I have implemented the tweak in ForceBalance to support current offxml v0.3, so all options are supported by ForceBalance, so the discussion here will not block the progress of the trial fitting.

By "supported" I mean the user is responsible for keeping the unit consistent in the file. This is true for openforcefield/data/test_forcefields/smirnoff99Frosst_experimental.offxml that I'm current using in fitting.

[jchodera]

Do we have an estimate for how much effort it would take to migrate FB to using a single API for manipulating force fields? It's generally good software design practice to localize all parts of an interface to something complicated (like a force field) through a level of abstraction, or else any changes to the underlying format will cause huge headaches. This is precisely what we are hoping to avoid by providing a simple, stable Python API to manipulate parameters via the toolkit.

[yudongqiu]

Do we have an estimate for how much effort it would take to migrate FB to using a single API for manipulating force fields?

Sorry I don't have a clear estimate for the amount of effort of this project. The current force field parser in ForceBalance not only provides information about the parameters, but also serves as an interface for the user to specify which parameters to include in fitting and their relations. Ditching this will require designing a new interface. Also it's hard to predict what might go wrong and how long it takes to fix them.

It's generally good software design practice to localize all parts of an interface to something complicated (like a force field) through a level of abstraction, or else any changes to the underlying format will cause huge headaches. This is precisely what we are hoping to avoid by providing a simple, stable Python API to manipulate parameters via the toolkit.

While I agree a stable Python API is great for sustainability in the future, I'm not sure if the current release has reached that point yet, because I guess API-breaking changes are still needed. On the other hand, the format of the force field file being reasonably stable is also very valuable. In terms of the current force field parser in ForceBalance, it will only require very little or no effort to maintain compatibility, as long as no major changes are made. (An example of a major change is replacing xml with an entirely new binary format.)

[leeping]

Hi there,

I wanted to chime in and argue that we should allow option (c) above to be used. This is principally because I think force field parameters of the same type should be given in the same unit system. I don't see a strong reason for wanting to specify individual units for each parameter, because it means that we either need to have a lot of repetition in specifying the same units for each parameter, or we would gain the ability to specify different units for parameters of the same type.

In terms of design philosophy, I think a file format should be designed such that "users are prevented from doing things incorrectly", which in my opinion includes specifying parameters of the same type in different units. Also, although I am in the minority, I think that file formats should be designed with simplicity in mind, in the sense that information should not be unnecessarily repeated to make things harder to read.

I don't see a compelling reason to specify different units for parameters of the same type; I think it would actually just lead to a sloppier file. I understand there were issues with entering the original parameters in the wrong units, but I don’t think it is a design flaw that the units were located in the header; we should make sure to look there next time. Moreover, this creates a big problem for FB because the prior widths (which has the same units as the parameter) is determined from the parameter name. If someone specifies one bond length in nanometers and another one in Angstroms, that will make it impossible for ForceBalance to do a good job in optimizing the force field.

I understand there are technical issues related to having properties common to all parameters in the header. I think that multiple parameters of the same type having “common properties” is actually a pretty fundamentally important thing, and we should be able to support property assignment hierarchies. Otherwise, in the future we could have a situation where each parameter holds many properties that are the same across all of the parameters, which will create major problems down the road. One example of this is anharmonic force constants for bonds where TINKER defines common values for all parameters. People will start to treat these common properties as things that can be changed individually, especially if such properties are floating point numbers.

Do we have an estimate for how much effort it would take to migrate FB to using a single API for manipulating force fields?

Because we are on such a tight time-table for producing release 1 of the force field, I don't think it's a great idea to spend major efforts on this. Details below.

First, the forcebalance.ForceField API is a core component of the code that affects how it works for everything including not just OpenFF but also AMBER, CHARMM, GROMACS, TINKER, and OpenMM XML files. You could even say ForceBalance already has a single API for manipulating force field files and it has been structurally stable for 5+ years.

So what is really being asked is for ForceBalance to adopt the OpenFF API for manipulating force fields, and the phrase "migrate FB to using a single API" implies FB should use the OpenFF API for everything else it currently does, i.e. it should also be used to manipulate the AMBER, CHARMM, GROMACS, TINKER force fields as well, not to mention basis set files for quantum chemistry. I agree with (a) in principle, but I have no idea how (b) could be accomplished and I actually disagree with it philosophically.

I understand there is now a greater incentive to adopt the OpenFF API because there was a performance regression in newer versions of the toolkit when the openforcefield.ForceField object is created from the XML file (it now takes ~5x longer); there was also the compatibility issue because the unit strings in new versions of the file caused problems with parsing. We already made changes in FB’s parsing code that admits the unit strings, but the parameter optimization is still affected by the performance regression. We’re currently looking at an overall ~2x slow-down in the optimization jobs compared with older versions of the toolkit.

We can look into using the OpenFF API to change parameters directly, but this is a major change to FB and would take time to test. I don't think it's a great idea to spend major efforts on this right now because our time table is to release a force field in August. Our efforts are currently being spent in other directions focused on putting the optimization job together (torsions, valence, integrating FB with property estimator, and gathering data). Meeting our deadline would be more assured if toolkit performance could be improved by creating the openforcefield.ForceField object from the XML file more rapidly, so that version 0.4 is not 5x slower than 0.3.

Finally, I was a bit shocked to learn about these changes because I wasn’t aware of them until after they were released. I guess it's partly my fault because the Github and Slack discussions were too numerous that I wasn't even aware of prior discussions, but they are having consequential impacts on being able to make our deadline now. If someone had asked me "hey, we're going to make this change so that it takes 5x longer to create a ForceField object, I would have said "don't do it, especially if we want to release a force field in August". If someone had asked me "should we allow different parameters of the same type to have different units", I would have opposed that too, for reasons given above.

Thanks,

• Lee-Ping

[davidlmobley]

Most of this will have to wait until Jeff is back, I think, but I just want to clarify a couple things...

Particularly @leeping note that you seem to be conflating two things:

1) Consistent units within each parameter type, and 2) Option (c), which attaches units to the category rather than to each parameter

It's not obvious to me that the only way to have option (1) is to also have option (2). Possibly there is a third option, which is to (3) require consistent units within each parameter type but still have them attached to the parameters rather than the category. In other words the parser could fail if the units are inconsistent.

The larger issues will need to wait for Jeff, I think, who is on the road. But I just want to be clear -- none of us wants people to mix units within a FF file:

I don't see a compelling reason to specify different units for parameters of the same type; I think it would actually just lead to a sloppier file.

That's not why we were doing this and we agree it would be sloppier for people to mix units. My understanding was that the main argument for this was for serialization/deserialization reasons.

@andrrizzi might also be able to comment though his formal involvement is diminishing. Or @jchodera .

[leeping]

Thanks, I agree the toolkit could throw an error if inconsistent units are found, and that would be a way to ensure consistent units without defining them in the header.

I still think a design that allows defining common properties in the header is desirable, because otherwise that could lead to a lot of repeated information and give the appearance that this information could vary for different parameters (unless the parser explicitly disallows it).

In other words, if we require all parameters to have some property in common (such as the unit system), I think the file format should reflect that. I also agree this should be considered in the context of the serialization-related issues, so we should wait for Jeff to come back to discuss more in depth.

[davidlmobley]

Right, I think the big issues we (all) want to deal with here are:

1) Serialization/deserialization should not be terrible 2) We want units attached to everything 3) We don't want it to be very slow 4) In the short term, we would like you not to have to do lots of extra work on ForceBalance under deadline, and ideally 5) We would rather not encourage users to use inconsistent units in the same parameter section anyway (and would be OK with disallowing it, I think), and especially not using inconsistent units would make it much harder to get ForceBalance to behave correctly in (4).

Hopefully we can separately deal with items (4) and (5) without having to undercut (1) and (2).

(In other words, there may be good ways we can all get what we need...)

[andrrizzi]

Jeff was more involved on serialization, but I'm quite sure that (c) will increase the complexity of the code, which means it's going to be harder to maintain and to implement new parameter types in the future. Moreover, this may result in weird situations if we end up implementing nested tags (which is still under discussion in #343).

As we discussed while making this decision, the choice is essentially a tradeoff between human-readibility (which I think it increases with (c)), and code complexity/extensibility (which is better with the other three options). Both are feasible. We just need to decide which aspect we want to prioritize.

As for the perfomance issue, @yudongqiu do you know what is its fundamental cause? There may be easy fixes we can think with a little more details. Apologies if I missed it while catching up on the discussion,

[leeping]

I agree with everything you said above and I'm okay with a compromise if needed. Ideally I'd like to see support for option (c) which would allow unit definitions and other common properties in the header. My viewpoint is more "file-centric" because FB has always manipulated force fields at the file level. If we require explicit units for each parameter and the toolkit checks that they're all the same, I'm okay with that too, but we should be aware of the future issues it might cause.

I'd mainly like to avoid a situation where all "common properties", including units, need to be defined at the individual parameter level. The other common property that I saw was idivf which is an integer, but other force types that we might want for the future could have larger numbers of common properties.

[yudongqiu]

@andrrizzi Thanks for following up on this discussion. For the performance issue I have just mentioned that to @j-wags before he went on vacation, and I agree it has a lot of potential to be improved. It's not urgent right now so I'm perfectly fine to work with @j-wags on this after he come back.

[jchodera]

@leeping : This whole argument about file formats is entirely missing the point: We are not, and will not be, designing file formats, period. We are designing data models, and providing an API for using and manipulating that data model. The format the data model is stored on disk is entirely irrelevant---we will eventually support a number of serialization formats for storage on disk and transmission over networks, and some of the representation decisions were made to ensure that this was possible.

But you should be using the Python API we are providing you for all manipulation of the force field. You should never operate on the file directly. It's bad design to operate on files, since the file representation may change, even though the data model does not. The reason we're having this discussion is because you chose to operate on the file level directly in spite of our warnings that this would change, and that the object model was the only way we should be operating on the data. That was fine for some quick-and-dirty testing, but now we need you to use the API.

[davidlmobley]

I think I have to agree with both of you/disagree with both of you, @leeping and @jchodera ...

It's going to take @leeping and @yudongqiu significant developer time to switch to using the API, we think. There may not be time to do that for release-1, which is why @j-wags remarked that if we have to, he can make a "spec-downgrader" they can use for release-1. HOWEVER: This would only be for release-1, at the MOST. As John remarked, we're not designing file formats, and we don't plan on necessarily providing a stable file format.

So I'd rephrase "now we need you to use the API" as "now we need you to begin planning to use the API, beginning no later than shortly after release-1, and maybe sooner."

It would be terrible if we had to delay release-1 simply because Lee-Ping's group had to spend a bunch of time switching to use the API before finishing their work on the release. But... we don't yet know how much time that would take, so it's hard to know if this is a realistic scenario.

That said, I think @yudongqiu should work with @j-wags on his return to come up with an estimate of how much developer time would be required to switch to the API. If @j-wags has to make a spec downgrader, that's a waste of developer time because he's literally making a one-off tool so that we can temporarily retain FB's ability to work on files. OTOH, if we get FB to use the API that's NOT a waste of time because it's something we have to do eventually. So the only reason to not switch FB to use the API now is if the developer time required for that is dramatically more than the developer time required to make a spec-downgrader, so much so that it would delay our release.

[jchodera]

So I'd rephrase "now we need you to use the API" as "now we need you to begin planning to use the API, beginning no later than shortly after release-1, and maybe sooner."

We did go through that process months earlier because we ForceBalance to switch to the API before now. I show records of our discussion of this in #smirnoff going back to Feb 8 as we prepared you to switch to the API to programmatically modify the parameters.

It sounds like the only workable approach for sticking with files in the short term is simply to assume the files we work with will have a highly rigid format, with a particular set of units specified, in the ForceBalance reader/writer for SMIRNOFF serialized files. This is still highly dangerous, but it could be very fast since you can ignore all the units and just manipulate the numbers (perhaps in OpenMM native units) directly. We can just agree on a specific set of uniform units to use for anything that goes into/out of ForceBalance. We don't need it to be flexible enough to accommodate any flavor of SMIRNOFF file.

[davidlmobley]

This sounds like a reasonable approach, yes, though "highly dangerous" since the machinery will only work as long as no one changes anything.

@leeping @yudongqiu ?

[leeping]

This sounds like a reasonable approach, yes, though "highly dangerous" since the machinery will only work as long as no one changes anything.

Yes, I think FB can continue to work around changes to the file formats, as long as they are not too major. The unit strings is not a major issue, but moving from XML to a pure binary format (as @yudongqiu mentioned earlier) would be a major issue. If we change the hierarchical design, i.e. <SMIRNOFF>/<HarmonicBondForce>/<Bond>, that would be a moderate issue but still workable.

The more major issues were the performance regression for creating openforcefield.ForceField objects from files, and my (mistaken) impression that we were designing the file to allow for mixing units.

But you should be using the Python API we are providing you for all manipulation of the force field. You should never operate on the file directly. It's bad design to operate on files, since the file representation may change, even though the data model does not.

Look, I'm all for working on stable things, but the fact is that files are physically stable representations of data. When people download our toolkit, files are what they get. If the file format is constantly changing then we lose this stability.

For example, if we are constantly changing our file representation so that a file written three months ago is no longer compatible with the current version of the toolkit, isn't that going to cause stability problems? For example we could decide to deprecate a certain version of the force field forever, and nobody will ever be able to use it again.

More generally, I'm unaware of any software that is constantly changing its file formats without attempting to preserve some kind of backwards compatibility.

[davidlmobley]

We ARE at this point preserving backwards compatibility, @leeping , in that newer openforcefield releases will be able to read older spec versions. But, new force field releases will be in the latest file format, which will continue to change as we add new features and (likely) realize that means we need to change some things about how the format is organized, etc. It's possible we may eventually even move to JSON, or at least also make JSON available.

The API, on the other hand, should be much more stable -- if you use it to retrieve a specific parameter you should get it back in a stable way regardless of underlying changes to the file format, and likewise for setting parameters.

Again, the plan is explicitly that the file format NOT be stable but that we (a) maintain ability to read old format files, and (b) provide a stable API so people who want to do things with the force field can be isolated from changes to the format.

[leeping]

Thanks for the explanations, and I'm fine with this plan.

Regarding our plans to migrate to using the OpenFF API: I think we could enable FB to change parameters using the API with some moderate changes to FB's code. FB currently manipulates parameters by storing a list of pointers to locations in the file. We can enable parameter modification through the API by storing a list of API calls instead of file locations. I think this should take 1-2 weeks of "focused" coding time if no major issues are encountered. That is certainly on our list of things to do, though it's not bound by the release-1 deadline.

If we change parameters using the API, it can improve performance in the sense that we won't need to be repeatedly creating openforcefield.ForceField objects, and there could be potential benefits for future performance improvements such as propagating parameter changes through createSystem and maybe even energy evaluations.

To be clear, this does not mean FB will completely avoid interacting with the files. It still needs to instantiate the forcebalance.ForceField object by reading the file to perceive its hierarchical structure and which parameters the user wants to change. So even if we use the OpenFF API to change the parameters, I don't think FB will ever become completely insensitive to the file format.

[jchodera]

The more major issues were the performance regression for creating openforcefield.ForceField objects from files, and my (mistaken) impression that we were designing the file to allow for mixing units.

We've said repeatedly that we can address the performance issue independently. The only way a major software effort like this can work is by decoupling pieces and allowing independent prioritization and optimization of things that fall on either side of a common simple API. You shouldn't have to worry about this beyond "we noticed a performance regression that is now a critical bottleneck for us---can you prioritize speeding this up?" I still don't see a separate issue filed on this, but if you could create one, we can prioritize addressing it.

In the best case, there are simple optimizations that will speed up things enormously. (@davidlmobley has already mentioned the top obvious possibility.) In the worst case, since we are now going to standardize the units we use in serialized force fields we interoperate with ForceBalance on, we could potentially even strip units completely and operate in an "unsafe" mode in which everything is in the OpenMM MD unit system, avoiding unit overhead altogether, but I'm hoping we won't have to do that.

Note also that, since FB is stuck in using files, there are likely other serialization formats that are much more performant than serializing/deserializing XML. But I think that since you operate on the file directly and do not serialize/deserialize these are not major issues for FB---but they could provide us with additional performance advantages for the toolkit later.

Regarding file format stability: We decided some time ago that the Python API would be the most stable aspect, and that we may need to make many rapid changes to the serialization formats before we reach a 1.0 stable version. We're getting close to that now, but there may still be further changes needed to the serialization format until we achieve all our needs of supporting multiple serialization formats. So bear with us!

[jchodera]

@leeping : I'm onboard what what you're saying about the benefits of eventually switching to using the Python API. If there are things we can add to the API to make the interface with ForceBalance easier, let us know. For example, if you want a flattened "view" of the numerical parameters so that FB can treat them as a vector of numbers, we can add that, since this would be helpful for any kind of parameterization scheme.

To be clear, this does not mean FB will completely avoid interacting with the files. It still needs to instantiate the forcebalance.ForceField object by reading the file to perceive its hierarchical structure and which parameters the user wants to change. So even if we use the OpenFF API to change the parameters, I don't think FB will ever become completely insensitive to the file format.

I think there may be better ways to specify which parameter subsets---perhaps we could discuss that on the slack?

[yudongqiu]

@jchodera @davidlmobley As I mentioned before, I have implemented small tweaks in ForceBalance to support option (a), so no spec downgrader is needed.

I think this discussion has been dragged to a completely different topic. Now we are spending a lot of time discussing what ForceBalance should do and it sounds like we're requesting to change the offxml format because of ForceBalance. This is not true because ForceBalance now supports all 4 options and I'm already running fitting with current spec v0.3. I think the discussion about how to refactor ForceBalance should be moved to the ForceBalance github repo.

I want to bring up the original topic again, that is I think the recent changes in the offxml format introduces an additional flexibility of allowing mixed units to be specified for parameters of the same type. I think this is unnecessary and expressed my concern to @j-wags, and we agreed to open this discussion here. I apologize if I didn't express this clearly, but the topic I have always wanted to focus on is "Should we remove this extra flexibility in offxml that allows different units for parameters of the same type?"

The answer to this question would have an impact on the this repo going into the future. As a user/collaborator of this package, I would like to suggest that we remove that "extra flexibility". In terms of how to do that, we can either enforce unit consistency in the toolkit with options (a) and (b), or we can use options (c) or (d) which naturally removes that "extra flexibility".

On the opposite, I think @jchodera suggested that we absolutely want no restrictions on the force field file format. That is achieved by enforcing all interactions with the force field through this toolkit, so we will use whatever file this toolkit serialized to as the file format. I think this also implies that there should be no problem even if we switch to use a binary format as the force field file.

If this "absolute freedom" on the force field file content is decided, then there is no point to have this discussion from the first place. @davidlmobley can you confirm? If it's not decided yet, I would like to vote against this "absolute freedom", because I think human readability is still very important, so it will be great if the content of the file is reasonably consistent throughout versions. And also, although it's hard to predict what might be needed in the future, I feel that a consistent force field file format has been beneficial for other MD packages. I think @j-wags has been doing a great job on keeping the offxml file content consistent even at this early development stage, and I hope we can keep doing this.

[davidlmobley]

@yudongqiu I don't think it's necessarily the case we want users to be able to mix and match different types of units within the same section (as long as they can pick and specify what units they use), but there could be serialization-related reasons that it would be hard to disallow this. Will have to discuss with Jeff on his return.

The things that are clear are that we want units attached, for serialization reasons we want them attached to the individual parameters, and ForceBalance should eventually move towards interacting with the API rather than the file itself.

[leeping]

I think we've worked through all of our technical issues, and this morning ForceBalance has been updated to use the OpenFF API for parameter modifications. :) We are seeing some good performance increases, so improving the performance of the constructor is no longer as crucial.

I still think that it would be better to have a file representation that minimizes redundancy, in the sense that the units should only be shown once if the toolkit only allows a single unit system to be used.

I can accept if this is technically not possible due to serialization issues, but I hope it is worth thinking about further. There are potentially many adjustable "common attributes" of parameters, such as idivf and other things down the road. Although I am not an expert on serialization, I want to avoid situations where all common attributes need to be replicated for each parameter in a section.

One more request for the future: I hope the file representation and the API can remain consistent in the handler and attribute names, because that makes it much easier for FB to modify parameters programmatically using the API.

[jchodera]

I still think that it would be better to have a file representation that minimizes redundancy, in the sense that the units should only be shown once if the toolkit only allows a single unit system to be used.

Eventually, we can add support for a compressed binary format that minimizes file or message size, but that's not a limiting factor right now.

One more request for the future: I hope the file representation and the API can remain consistent in the handler and attribute names, because that makes it much easier for FB to modify parameters programmatically using the API.

For human-readable serializations, yes, it would be ideal to be consistent in naming.

[j-wags]

Just catching up on this today. Apologies for starting a big discussion and then stepping out. I agree with much of what's been said, and actually I'm really glad that we're bringing all of this up early. I suspect that the lasting developments in our field came from big-picture discussions like this (SMILES, anyone?), so I'm glad were being intentional about our choices here. Also, many of these points will generalize to future decisions about serialization and toolkit user experience (looking at you, FrozenMolecule.to_file()!), so we're likely to benefit from this discussion past the current issue.

So my understanding of the conversation so far is:

Decisions from this discussion:

• I'll prioritize toolkit performance improvements that are accounting for a significant fraction of our optimization time. (I didn't even realize there was such a large performance hit with the 0.4.0 release! 😱 ) Right now the toolkit isn't runtime-optimized for any particular purpose, and I suspect we can achieve orders-of-magnitude speedup as needed, so let's stay in close contact if the toolkit continues to be a significant percentage of FB runtime.
• We'll implement direct correspondences between XML attribute names and python attribute names: https://github.com/openforcefield/openforcefield/pull/377 (This will be a ForceField API design principle moving forward)

Potential decisions from this discussion:

• As @jchodera has mentioned before, we might offer a "view" of all the parameters as a numerical array, and also functions to standardize the unit systems in an FF (if we continue to allow different units).

Remaining to be decided: 1) Should inconsistent units be allowed in parameter sections? 2) In the serialized format, do we use (a), (b), (c), or (d) from above?

These two choices are entangled. If we choose anything except "only one unit system is valid" and (d), then we need to define "unsurprising" behavior for the units that would be written out after a ForceField is manipulated in the following ways:

• a script reads two OFFXMLs with <Bond>/'k' in different units and tries to write out the merged ForceField. Which units should come out?
• Same as above, but if the two OFFXMLs were loaded in the opposite order.
• A script loads an OFFXML, then uses the API to set a single <Bond>/k value in a different unit system than was read in.
• Same as previous, but the modified parameter is then deleted.
• An OFFXML is loaded, and the user adds a k#/periodicity#/phase# value to an existing torsion in a different unit system.

A few months ago, I was trying to implement sane behavior for these situations using the 0.2 spec, which is (c). I started going pretty deep into the decision tree rabbit hole for "sensible behavior" in those situations, and a few forks in, I realized that things were getting very complex very fast. I was going to need to shut down or override access to ParameterType objects so that the owning ParameterHandler could record when users accessed their values and whether they changed the unit. A ParameterType object became incapable of serializing itself, because it needed to hear from the ParameterHandler to tell it which units to use. The ParameterHandler had to keep a record of which parameters were added or modified, using which units, in which order, and how to backtrack in history if the most recent was deleted. Basically, the data structure became super brittle and interconnected. And for all the added complexity, there was still a very real chance of user surprise. And I didn't know how to enumerate the possible sequences of events that could precede serialization, so I wasn't sure if I had covered all the bases. So, that was the experience that led me away from (c).

Basically, any solution where we allow different units will lead to this problem. Even if we enforce that units must be identical within one section, it is still a core part of the spec that sections from different sources will be combined if both are read, and then we're back to the decision tree.

So, I think (a), (b), and (d) are the OK options in terms of code complexity.

I'm not a huge fan of (b) because it adds an interdependence of parameter names (the X value needs to have a correctly-spelled X_unit tag adjacent). It would also send a very confusing error message if someone tried to add a cosmetic attribute ending in _unit (in our field, crystal_unit might be a real danger). It's unlikely to be a problem, but it's also added complexity that isn't strictly necessary.

(d) is the other option I'd consider, but wouldn't prefer. It would be a large reduction in complexity, but also some significant reduction in user-friendliness. People converting force field parameters from other sources will benefit substantially from being able to match exact values with the original source (and unit system). It will aid in provenance and catching bugs for those folks. And I think that, strategically, OFF should err on the side of being nice and explicit for those converting their FFs to SMIRNOFF, since they're the people we need to convince to join our community.

So, I'm still in support of (a). Basically, I think human attention/focus is expensive, mistakes are really bad, and disk space is cheap. If we can triple the file size but reduce the chance of making a mistake by 1%, I see that as a great tradeoff.

[jaimergp]

[...] and disk space is cheap.

And if that really becomes an issue, these repetitive patterns would greatly benefit from standard compression schemes (.tar.gz etc).

[mattwthompson]

More than three years have passed now, which in all likelihood renders some of the details here out of date with how much our code changes. If there are existing issues around serialization, performance, etc. please do not hesitate to raise a new issue with updated information.