0.4.0 releasenotes #341

j-wags · 2019-06-01T20:05:54Z

This PR is intended to allow review and discussion of the 0.4.0 release notes.

Update changelog

codecov-io · 2019-06-03T20:17:40Z

Codecov Report

Merging #341 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     openforcefield/openff-toolkit#341   +/-   ##
=======================================
  Coverage   75.89%   75.89%           
=======================================
  Files          17       17           
  Lines        5418     5418           
=======================================
  Hits         4112     4112           
  Misses       1306     1306

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6baf438...7ababc7. Read the comment docs.

codecov-io · 2019-06-03T20:17:40Z

Codecov Report

Merging #341 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     openforcefield/openff-toolkit#341      +/-   ##
==========================================
+ Coverage   75.89%   75.93%   +0.03%     
==========================================
  Files          17       17              
  Lines        5418     5427       +9     
==========================================
+ Hits         4112     4121       +9     
  Misses       1306     1306

Impacted Files	Coverage Δ
openforcefield/utils/utils.py	`86.53% <0%> (+0.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6baf438...e25596f. Read the comment docs.

andrrizzi

Looks great to me!

andrrizzi · 2019-06-04T10:48:20Z

docs/releasehistory.rst

+  This requirement can be skipped by providing the kwarg ``skip_version_check=True``
+* ``ParameterType`` and ``ParameterHandler`` functions no longer handle ``X_unit`` attributes in SMIRNOFF data (per the SMIRNOFF spec change above).
+* The scripts in ``utilities/convert_frosst`` are now deprecated.
+  This functionality is importance for provenance and will be migrated to the ``openforcefield/smirnoff99Frosst`` repository in the coming weeks.


typo importance -> important

jchodera

Apologies for all of the comments, but I think we're getting better at this slowly but surely!

jchodera · 2019-06-05T08:53:26Z

docs/releasehistory.rst

+0.4.0 - SMIRNOFF Spec Update and Performance Optimization
+---------------------------------------------------------
+
+This update introduces the 0.3 SMIRNOFF spec.


"SMIRNOFF 0.3 spec" is easier to find.
Also, you should link to the spec here.

Reworded and link added.

jchodera · 2019-06-05T08:53:54Z

docs/releasehistory.rst

+---------------------------------------------------------
+
+This update introduces the 0.3 SMIRNOFF spec.
+This spec upgrade is the result of discussions about how to handle the evolution of data and parameter types as further functional forms are added to the SMIRNOFF spec.


It's not an "upgrade", but an "update", "extension", or "revision".

Changed to "update"

jchodera · 2019-06-05T08:55:24Z

docs/releasehistory.rst

+This update introduces the 0.3 SMIRNOFF spec.
+This spec upgrade is the result of discussions about how to handle the evolution of data and parameter types as further functional forms are added to the SMIRNOFF spec.
+
+The toolkit now contains functionality to "upgrade" the spec of a data source.


This is unclear.
Perhaps "We provide scripts to migrate SMIRNOFF 0.1 and 0.2 forcefields written with the XML serialization (.offxml) to SMIRNOFF 0.3 XML serializations.

Good call. Added.

jchodera · 2019-06-05T08:56:06Z

docs/releasehistory.rst

+
+The toolkit now contains functionality to "upgrade" the spec of a data source.
+These methods are called automatically when loading an 0.1 or 0.2-spec data source.
+This functionality allows the toolkit to continue to read 0.2-spec files, and also implements backwards-compatibility for 0.1-spec files.


I'd be consistent about using "SMIRNOFF 0.2 XML serialization files", or some other consistent way of describing

it's SMIRNOFF 0.2

it's the XML serialization

Changed to This functionality allows the toolkit to continue to read files containing SMIRNOFF 0.2 spec force fields, and also implements backwards-compatibility for SMIRNOFF 0.1 spec force fields.

jchodera · 2019-06-05T08:56:30Z

docs/releasehistory.rst

+This functionality allows the toolkit to continue to read 0.2-spec files, and also implements backwards-compatibility for 0.1-spec files.
+
+
+WARNING: The 0.1 SMIRNOFF spec did not contain fields for several energy-determining parameters that are exposed in the 0.2 spec.


"SMIRNOFF 0.1", not "0.1 SMIRNOFF".
"SMIRNOFF 0.2", not "0.2 spec".

jchodera · 2019-06-05T08:58:33Z

docs/releasehistory.rst

+If you use the spec conversion functionality for other force field families, please carefully review the warning messages printed during spec conversion to ensure they are providing your desired behavior.
+
+
+This update also greatly improves performance when running ``create_openmm_system`` on large topologies.


The note about this performance optimization might come first so it is not lost in the complex description of the update process above.

Good idea. Moved to the beginning.

jchodera · 2019-06-05T08:59:02Z

docs/releasehistory.rst

@@ -7,6 +7,60 @@ Releases follow the ``major.minor.micro`` scheme recommended by `PEP440 <https:/
 * ``minor`` increments add features but do not break API compatibility
 * ``micro`` increments represent bugfix releases or improvements in documentation

+
+
+0.4.0 - SMIRNOFF Spec Update and Performance Optimization


Should be: "0.4.0 - Performance optimizations and support for SMIRNOFF 0.3 specification"

Added as written.

jchodera · 2019-06-05T08:59:12Z

docs/releasehistory.rst

+
+This update also greatly improves performance when running ``create_openmm_system`` on large topologies.
+
+SMIRNOFF Spec Changes


SMIRNOFF 0.3 specification updates

Also, link to the SMIRNOFF 0.3 specification.

I worry we're going to run into trouble here where there will be confusion between the global SMIRNOFF specification version, the top-level XML tag version, and individual parameter level versions. I think we have to clarify to the reader how these three versions are going to be separately updated:

Will each update or extension to the SMIRNOFF spec increment the spec number?

Will this also increment the top-level XML tag version, or does that only change when we add a new aromaticity model or something?

How are the individual parameter level version numbers to be incremented now?

If we can clarify all of this to the reader, it will help!

Here are my thoughts for what we could include in the release notes:

We update the section versions when:

A new section attribute is exposed or an existing one is removed

A new section attribute value becomes accepted (for example, a new "named" potential type like "harmonic" or "charmm")

We change the behavior of a section, but nothing about its syntax

We change a default value in a section, but nothing about its syntax

We update the SMIRNOFF version when:

We add a new supported aromaticity model

We add a new top-level attribute (it's hard to think of an example of these... maybe we find a limitation with SMIRKS and create a chemical_perception="SMIRKS1" or "SMIRKS2" option that users can switch between)

We update the XML version when:

Not within the spec/Not applicable. This is out of our control. The modular nature of ParameterIOHandlers means that users are free to serialize in any format they like, as long as the proper data ends up getting passed to the ForceField constructor (currently the "SMIRNOFF data dict", which is sort of a loose end but will stabilize as time goes on).

Here are some unresolved questions we may need to solve

What is the relationship between a parameter section and the SMIRNOFF tag?

I actually don't know, so I'm going to "think out loud" here.

The SMIRNOFF tag currently encodes the aromaticity model. Could we achieve the same behavior by having each parameter section have an aromaticity model? Yes, we could.

So maybe, the idea of an enclosing tag in the SMIRNOFF spec indicates "a value that would otherwise need to be set repeatedly in the enclosed tags". In that sense, the top-level aromaticity_model attribute is implicitly a value that affects, for example, the BondHandler's behavior.

As a thought experiment, I could make a new ParameterHandler that applies parameter based on a different aromaticity model, and have it override the higher level aromaticity model. This is a weird example, but I couldn't come up with a better one -- The point is that it would seem to be arbitrary to tell a developer they can't do that, just because we say so.

Note that the above "sections should be enclosed in other sections if the enclosing section affects the behavior of the enclosee" logic would have a consequence for @andrrizzi's question in #310 (comment)

First thing that comes to mind is that we might want to have the charge model tags being children tags of Electrostatics, which will essentially have 3 levels of indentation instead of 2.

It implies that the sections that describe atomic charges should be enclosed by the Electrostatics tag (since attributes like the cutoff influence the functional form). It also opens the question of "what if a person wanted both the Electrostatics and GBSA tags? What should enclose the charge-determining sections?". I don't know what we'd do in that situation.

And, in a similar vein, this will make things like VirtualSites more difficult, since they currently are in their own section, but will experience electrostatics and vdW forces. Should they be doubly-nested in Electrostatics and vdW tags? Or should the VirtualSite definitions be duplicated in the enclosing Electrostatics and vdW sections?

So maybe this isn't the right view of section enclosure.

Maybe the SMIRNOFF version encodes something about the underlying section structure

For example, in the 0.2 -> 0.3 transition, we switch from units-in-the-header to units-in-the-quantity. Well, I'd argue that even that change could have been handled at the parameter section level. So one could argue that changing to the "SMIRNOFF 0.3 spec" doesn't actually mean anything -- it's the parameter sections that need to change their behavior! But, I guess the SMIRNOFF spec change here records a "rule change", because the SMIRNOFF 0.3 spec allows ParameterHandlers with their own versions.

Is a parameter section's version truly just the version=Y attribute in its defining tag? Or is it really a tuple of (SMIRNOFF_version=X, section_version=Y)?

I think it has to be the tuple, because if a parameter section only works with SMIRKS1, but the SMIRNOFF tag sets SMIRKS2... then the parameter section needs to know about this higher-level setting that didn't even exist when it was written. So I guess each parameter section will need to have a MAX_SMIRNOFF_VERSION tag. And probably a MIN_SMIRNOFF_VERSION too.

@jchodera @andrrizzi the first part of the post above is relevant to merging this PR. The bottom part can continue to be discussed but isn't blocking.

Migrated general part of the post to #120

I think we can consider using the top-level XML tag version as the "global" version that defines to the general rules for building a SMIRNOFF XML document

Maybe I'm misunderstanding, but I think we're going the wrong direction by thinking about the XML version here. We will serialize in versions other than XML, so having the XML version have any bearing on the meaning of a SMIRNOFF file is probably bad.

I'd argue that any "general rules" about structure need to be encoded by the SMIRNOFF version. I could be convinced that we need a second "structure" version, but I'm having trouble thinking broadly enough to know what sort of change that might encode.

I think I expressed myself confusingly. What I was trying to say there is that I think we need only two types of version: the one specified in the SMIRNOFF root tag, which is associated to the general grammar of the document, and the children tag version. Possibly, there will be multiple children tag versions that are compatible with the same SMIRNOFF root tag version.

I agree that that the code only needs to concern itself with the root tag and section tag versions!

For the future, I think we were just trying to figure out if the "official SMIRNOFF spec" should also have a global version that encodes the most up-to-date collection of these root and section tag versions, or if this is not necessary.

I see, thanks for the clarification. So essentially this would be versioning a "recommended" combination of tags and the GitHub documentation page? I can see the convenience of grouping a collection of versions into a single number for citation purposes. On the other hand, adding yet another version could generate some confusion.

I tend towards not adding right now it simply because I think people will have to cite a full offxml file(s) anyway, which already implies a collection of versions so I don't see a strong use case to support the added confusion, but I don't have a strong opinion about it, and I'll be happy with any decision you and Jeff will settle on.

jchodera · 2019-06-05T08:59:55Z

docs/releasehistory.rst

+
+SMIRNOFF Spec Changes
+"""""""""""""""""""""
+* All individual sections are now versioned.


SMIRNOFF 0.3 introduces versioning for each individual parameter section, allowing asynchronous updates to the features of each parameter class.

Added as written

jchodera · 2019-06-05T09:04:22Z

docs/releasehistory.rst

+Adds the following new functions:
+
+* `PR #311 <https://github.com/openforcefield/openforcefield/pull/311>`_:
+    * ``utils/utils.py``: Adds ``convert_0_2_smirnoff_to_0_3``, which takes a 0.2-spec SMIRNOFF data dict, and upgrades it to 0.3.


Why describe what file it's in? Nobody will use it that way. They'll either use the entry point script (if you provide one) or the Python import path. Explain which are available.

Also, use consistent naming: "0.2-spec SMIRNOFF" should be "SMIRNOFF 0.2".

Also, the "data dict" structures are very dangerous. We DON'T want people using these. I'd add warnings that these are not guaranteed to be stable forever.

Same goes for all the features listed below.

Good call. Removed the file paths.

Changed references to SMIRNOFF spec per your suggestion.

Added:

NOTE: The format of the "SMIRNOFF data dict" above is likely to change significantly in the future. Users that require a stable serialized ForceField object should use the output of ForceField.to_string('XML') instead.

Changed the to "PR #311: Several new experimental functions." to make it clear that these aren't stable.

j-wags · 2019-06-06T04:09:39Z

I really appreciate the detailed feedback, @jchodera.

I realized that maybe I don't know how many things are being versioned here. For sure the top-level SMIRNOFF tag and parameter sections are, but is the entire file? Will we ever deviate from xml version='1.0'/ is there a hidden assumption about forcefield structure on that level? I'd initially thought that the independent versioning of the parameter sections and the top-level SMIRNOFF tag made those two numbers completely defining for the file contents, so I had actually removed the version number from the title line of the SMIRNOFF spec docs, but now I'm thinking that was premature.

I'll rethink this question in the morning. Thanks again for your close read of this.

jchodera

This is great! You've made the release notes clear, succinct, and precise. Thanks so much for taking such care in this revision!

I've noted a few minor things---should be quick to fix.

jchodera · 2019-06-07T14:39:29Z

docs/releasehistory.rst

+0.4.0 - Performance optimizations and support for SMIRNOFF 0.3 specification
+----------------------------------------------------------------------------
+
+This update improves runtime when running ``create_openmm_system`` on large topologies.


Unclear what is meant by "improves runtime". Perhaps:

"This update contains performance enhancements that significantly reduce the time to create OpenMM systems for topologies containing many molecules" via ForceField.create_openmm_system()?

Looks good. Added as written.

jchodera · 2019-06-07T14:39:40Z

docs/releasehistory.rst

+The spec update is the result of discussions about how to handle the evolution of data and parameter types as further functional forms are added to the SMIRNOFF spec.
+
+
+We provide methods to convert SMIRNOFF 0.1 and 0.2 forcefields written with the XML serialization (.offxml) to the SMIRNOFF 0.3 XML specification.


Quote-protect .offxml

jchodera · 2019-06-07T14:39:58Z

docs/releasehistory.rst

+This functionality allows the toolkit to continue to read files containing SMIRNOFF 0.2 spec force fields, and also implements backwards-compatibility for SMIRNOFF 0.1 spec force fields.
+
+
+WARNING: The SMIRNOFF 0.1 spec did not contain fields for several energy-determining parameters that are exposed in later SMIRNOFF specs.


This should be a .. warning :: block.

Changed for this release.

Generally, I'm nervous about putting in too much ReStructuredText-specific formatting here if it's not absolutely necessary. Since we will be copying these to the GitHub release text (which uses MarkDown), each element that needs reformatting adds a little avoidable complexity.

Are there RST-specific features in this document that we couldn't afford to lose, or could we convert the releasenotes to markdown?

jchodera · 2019-06-07T14:41:00Z

docs/releasehistory.rst

+Performance improvements
+""""""""""""""""""""""""
+
+* `PR #329 <https://github.com/openforcefield/openforcefield/pull/329>`_: Improved runtime for system creation in large topologies.


"Improved runtime" -> "Performance improvements". And does "large" refer to many atoms or many molecules?

"large" refers to "many atoms". Changed to:

Performance improvements when creating systems for topologies with many atoms.

jchodera · 2019-06-07T14:46:41Z

docs/releasehistory.rst

+
+
+
+API-breaking changes


Is it possible to have these functions described below have links that resolve to the API docs using Sphinx cross-referencing?

jchodera · 2019-06-09T07:10:24Z

Looks like this might need to be rebased?

j-wags added 2 commits June 1, 2019 13:04

First draft of 0.4.0 releasenotes

8b3cde7

releasenotes wording change

7ababc7

Small updates to release notes

f51c920

j-wags changed the title ~~First draft of 0.4.0 releasenotes~~ 0.4.0 releasenotes Jun 4, 2019

j-wags requested a review from andrrizzi June 4, 2019 00:09

andrrizzi approved these changes Jun 4, 2019

View reviewed changes

j-wags added 2 commits June 4, 2019 09:38

importance-->important

a3893cd

Revise release notes

f14e025

jchodera requested changes Jun 5, 2019

View reviewed changes

Revise release notes

91a4d03

Standardize on "SMIRNOFF 0.X specification" wording

7da2e9d

j-wags mentioned this pull request Jun 7, 2019

Container class for parameterized systems #310

Closed

andrrizzi mentioned this pull request Jun 7, 2019

Long term sustainability of ParameterHandler #343

Open

jchodera approved these changes Jun 7, 2019

View reviewed changes

j-wags mentioned this pull request Jun 7, 2019

Address open questions on SMIRNOFF format spec revamp openforcefield/standards#48

Open

j-wags added 4 commits June 7, 2019 08:08

test adding sphinx documentation link, small text changes

7ef7c5e

Adding method API links, part 1

6af8f0b

Adding class/method API links, part 2/2

f3dd3b0

small fixes

e25596f

j-wags added 3 commits June 10, 2019 09:16

Merge branch 'master' into 040_releasenotes

f611ce3

adds charge assignment bugfix

6cd149a

Removed dangling backtick

7ab7202

j-wags merged commit 4623297 into master Jun 10, 2019

j-wags deleted the 040_releasenotes branch June 10, 2019 18:50

		This functionality allows the toolkit to continue to read 0.2-spec files, and also implements backwards-compatibility for 0.1-spec files.


		WARNING: The 0.1 SMIRNOFF spec did not contain fields for several energy-determining parameters that are exposed in the 0.2 spec.

		If you use the spec conversion functionality for other force field families, please carefully review the warning messages printed during spec conversion to ensure they are providing your desired behavior.


		This update also greatly improves performance when running ``create_openmm_system`` on large topologies.


		This update also greatly improves performance when running ``create_openmm_system`` on large topologies.

		SMIRNOFF Spec Changes

		The spec update is the result of discussions about how to handle the evolution of data and parameter types as further functional forms are added to the SMIRNOFF spec.


		We provide methods to convert SMIRNOFF 0.1 and 0.2 forcefields written with the XML serialization (.offxml) to the SMIRNOFF 0.3 XML specification.

		This functionality allows the toolkit to continue to read files containing SMIRNOFF 0.2 spec force fields, and also implements backwards-compatibility for SMIRNOFF 0.1 spec force fields.


		WARNING: The SMIRNOFF 0.1 spec did not contain fields for several energy-determining parameters that are exposed in later SMIRNOFF specs.




		API-breaking changes

0.4.0 releasenotes #341

0.4.0 releasenotes #341

Conversation

j-wags commented Jun 1, 2019

codecov-io commented Jun 3, 2019

Codecov Report

codecov-io commented Jun 3, 2019 • edited Loading

Codecov Report

andrrizzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchodera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Here are my thoughts for what we could include in the release notes:

We update the section versions when:

We update the SMIRNOFF version when:

We update the XML version when:

Here are some unresolved questions we may need to solve

What is the relationship between a parameter section and the SMIRNOFF tag?

Maybe the SMIRNOFF version encodes something about the underlying section structure

Is a parameter section's version truly just the version=Y attribute in its defining tag? Or is it really a tuple of (SMIRNOFF_version=X, section_version=Y)?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-wags commented Jun 6, 2019 • edited Loading

jchodera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchodera commented Jun 9, 2019

codecov-io commented Jun 3, 2019 •

edited

Loading

Is a parameter section's version truly just the `version=Y` attribute in its defining tag? Or is it really a tuple of `(SMIRNOFF_version=X, section_version=Y)`?

j-wags commented Jun 6, 2019 •

edited

Loading