Wrong "scale_14" read from .json file about openff-interchange HOT 5 CLOSED

Thanks for finding this - I can reproduce it and it's totally valid

That part of the JSON blob is parsed here, where something is going awry when re-instantiating the object.

It's not the raw JSON parsing, which finds the right value, but something in .parse_raw is not iterating over the keys and mapping them onto fields like I'd expect

In [17]: json.loads(i["Electrostatics"].json())["scale_14"]
Out[17]: 0.8333333333

In [18]: SMIRNOFFElectrostaticsCollection.parse_raw(i["Electrostatics"].json())
Out[18]: Handler 'Electrostatics' with expression 'coul', 11 mapping keys, and 11 potentials

In [19]: SMIRNOFFElectrostaticsCollection.parse_raw(i["Electrostatics"].json()).scale_14
Out[19]: 0.5

Interchange has a custom loader, but this particular collection does not. It falls back to this function, which ... maybe isn't parsing numbers at all? I think this line is indented over one too far

The quickest fix for this behavior would be to set the default value to the traditional value of round(5 / 6, 10) - notice how the class definition doesn't override the default provided by its base class(es).

In [25]: SMIRNOFFElectrostaticsCollection().scale_14
Out[25]: 0.5

Obviously a more robust fix would go onto the base class's JSON parsing function. To be honest, the JSON roundtripping of Interchanges (and their constituent components) is neither well-tested nor broadly-used yet, so there are probably more than a few of these bugs out there. One day I'll add a bunch of tests that just round-trip everything with JSON from a variety of different states ....

Anyway, happy to review PRs for any combination of these ideas if you wanted to take a stab at them and if my suggestions are coherent. You're under no obligation to, of course, as this is all behavior that a user should expect to work.

from openff-interchange.

Here's the sort of test I would like added en masse one day¹: using only slight extensions of your snippet:

from openff.toolkit import Molecule, ForceField, Quantity
from openff.interchange import Interchange
from openff.interchange.drivers import get_gromacs_energies

def test_issue_908():
    m = Molecule.from_smiles("CCC")
    m.generate_conformers()
    t = m.to_topology()
    t.box_vectors = Quantity([4, 4, 4], "nanometer")
    
    ff = ForceField("openff-2.1.0.offxml")
    
    i = ff.create_interchange(t)
    
    with open("test.json", "w") as f:
        f.write(i.json())
    
    j = Interchange.parse_file("test.json")
    
    get_gromacs_energies(i).compare(get_gromacs_energies(j))
    # FAILED test.py::test_issue_908 - openff.interchange.exceptions.EnergyError: {'Electrostatics': <Quantity(-0.29432869, 'kilojoule / mole')>}

I think there are some basic tests for JSON round-tripping, just not one that looks at this particular comparison

from openff-interchange.

@pbuslaev try the main branch now - not sure when the next release will be

from openff-interchange.

@mattwthompson Thanks a lot! Looks super cool.

Related question, I would like to use Interchange combination functionality. Do you want me to add test cases for it?

from openff-interchange.

Absolutely!

The only detail you should be aware of is that I'm planning on Interchange.__add__ going away and it being replaced by some sort of Interchange.combine. The business logic should be similar, but having it be a concrete method allows for arguments to be passed in and for the possibility that something could be returned (i.e. some sort of atom mapping between input and final states).

from openff-interchange.