Fix missing sigma/rmin_half attrs

[mattwthompson]

#537

TODO:

• Fix failing virtual site tests
• More dict tests
• Possibly switch default rep to display sigma, despite our force fields being shipped with rmin_half

Boilerplate:

• Tag issue being addressed
• Add tests
• Update docstrings/documentation, if applicable
• Lint codebase
• Update changelog

[j-wags]

Possibly switch default rep to display sigma, despite our force fields being shipped with rmin_half

Unfortunately, I think we're in a tough spot, since the most fundamental thing the toolkit should support is faithful round-tripping of OFFXML files. So I don't think a solution that converts rmin_half to sigma in a round trip is ok here. I like the functionality to retrieve either sigma or rmin_half from the getters, but to_dict needs to output whichever of the two was set most recently.

The only solution I can think of is a hidden attribute that records "which representation was most recently set" for each vdWType, or having the sigma setter clear the rmin_half attribute and vice versa.

[mattwthompson]

I can't say I find it to be a satisfying solution, but I also can't think of anything better, and this requires the least sweeping changes to the codebase as is.

the most fundamental thing the toolkit should support is faithful round-tripping of OFFXML files

Could you expand on the details a little bit? What are the exact requirements for this condition to be satisfied? This appears to currently be be tested, although I'd add that a naive check (below) and the existing serialization tests in TestForceField pass for all commits of this branch, so I'm not sure that

So I don't think a solution that converts rmin_half to sigma in a round trip is ok here.

is a requirement that's explicitly checked in the tests. I did one test by hand (with recent changes) and the mainline files are exported with rmin_half, the only differences between the files being what you'd expect (comments, cosmetic attributes, etc.).

In [1]: from openforcefield.typing.engines.smirnoff import ForceField


In [2]: parsley = ForceField('openff-1.3.0.offxml')

In [3]: parsley.to_file('out.offxml')

In [4]: round_trip = ForceField('out.offxml')

In [5]: parsley == round_trip  # this checks id(obj) so it should never pass
Out[5]: False

In [6]: hash(parsley) == hash(round_trip)
Out[6]: True

[codecov]

Codecov Report

Merging #750 (71f2277) into master (b9d499f) will increase coverage by 0.01%.
The diff coverage is 95.65%.

[j-wags]

This looks good, and I couldn't find a way to confuse this system while playing around locally. Good work implementing a significant new behavior into the existing codebase in like, 30 lines. That's really impressive.

I've added a suggestion that should bring your line count down lower. Can't let you pad those github stats :-P

Be sure to update the releasenotes before you merge!