Fix issue #159

[lohedges]

This PR closes #159. On conversion to OpenMM format we set a shared space property if one isn't already present. If there isn't one, then we use an infinite Cartesian space. On read, systems are automatically assigned a shared space property, since we wrap the new sr.load code. In order to preserve the existing BioSimSpace behaviour related to extraction and recombination of molecules, I've needed to remove these shared properties, adding them back as regular system properties. This avoids situations where multiple molecules in a new system have a different space property, or an incompatible space property being applied to a system on conversion to OpenMM when a vacuum simulation would be expected.

I've also updated the code to use a shared property on read for the fileformat. (Previously this was done in a manual way.) This is useful so that extracted molecules passed on to certain parameterisation functions retain information that specifies the format from which they were loaded, i.e. if SDF then we can safely write back to the same format in knowledge that all information is preserved.

• I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
• I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

[chryswoods]

This looks like a good solution that preserves existing behaviour. There is a lot of subtlety with respect to whether or not to get the space out when extracting molecules from systems. I ended up using the shared space as it was less surprising when energies were calculated between two molecules extracted from the same system (they used the shared periodic space, and so minimum image wrapped to be close together, rather than using an infinite space and being far apart).

It is really hard to know if someone is expecting a space to be preserved or for it to be lost / reset when pulling out molecules. I think we need to think about it for the future.

Perhaps something like a test when molecules are added, that they use the shared space if their space values are identical, but reset to an infinite space if their spaces are different.

In all cases, passing map={"space": YourDesiredSpace} would let the user explicitly set the space they want to use.

[lohedges]

Thanks, I agree that it would be good to revisit it. I was also thinking about checking for a consistent space across molecules but thought that it might be slow in the Python layer. There are also bits of functionality behind the scenes where molecules are removed and readded to an existing system, e.g. waters, so didn't want any multi molecule space check to break anything here.

[chryswoods]

I agree - we want a C++ function or something that can set a consistent space, e.g. sire.mol.set_consistent_space(mols) or similar.