search

openforcefield / openff-evaluator

A physical property evaluation toolkit from the Open Forcefield Consortium.
https://docs.openforcefield.org/projects/evaluator
MIT License
55 stars 18 forks source link

Hvap calculations broken #450

Closed jthorton closed 2 years ago

jthorton commented 2 years ago

I have found that when trying to repeat the evaluator example in forcebalance here the calculations fail with an error related to openff.units and a message in the logs about the equilibration_simulation_gas protocol failing to execute

aeec2c29c7254d44a08924d9d3cd5930|equilibration_simulation_gas failed to execute.

Traceback (most recent call last):
  File "/home/josh/mambaforge/envs/nagl/lib/python3.8/site-packages/openff/evaluator/workflow/protocols.py", line 1245, in _execute_protocol
    protocol.execute(directory, available_resources)
  File "/home/josh/mambaforge/envs/nagl/lib/python3.8/site-packages/openff/evaluator/workflow/protocols.py", line 704, in execute
    self._execute(directory, available_resources)
  File "/home/josh/mambaforge/envs/nagl/lib/python3.8/site-packages/openff/evaluator/protocols/openmm.py", line 519, in _execute
    openmm_pressure = to_openmm(pressure)
  File "/home/josh/mambaforge/envs/nagl/lib/python3.8/site-packages/openff/utilities/utilities.py", line 75, in wrapper
    return function(*args, **kwargs)
  File "/home/josh/mambaforge/envs/nagl/lib/python3.8/site-packages/openff/units/openmm.py", line 132, in to_openmm
    value = quantity.m
AttributeError: 'NoneType' object has no attribute 'm'

I think as the Hvap calculations require a gas phase simulation which sets the pressure to None this causes an issue when we try and convert the value to openmm compatible units here.

Maybe something like this would work

 openmm_pressure = None if self.ensemble == Ensemble.NVT else to_openmm(self.thermodynamic_state.pressure)
mattwthompson commented 2 years ago

Is this using the master branch or a released version? That branch is unfortunately in a half-broken state. I'm maintaining a 0-3-x that's compatible with the openff-toolkit < 11.0.

Right now I don't think much has been added since the last release (v0.3.11), but if there is any reason to make one we can (#447).

jthorton commented 2 years ago

This is with 0.3.11 but as you say it looks very similar to main and I believe will be broken in that as well. I am happy to test out a fix and get a PR in though! Maybe with this and #451 we could look at a new release?

mattwthompson commented 2 years ago

Okay hmm, I see now. A little confused as I'm not as familiar with Evaluator as I should be but

happy to test out a fix and get a PR

I'd gladly take you up on this and get look to these changes into a new release

mattwthompson commented 2 years ago

self.thermodynamic_state.pressure should now already be an OpenFF Quantity: https://github.com/openforcefield/openff-evaluator/blob/c21faba6f3ece8ef7c19bd837e4dc5f1d0eefb69/openff/evaluator/thermodynamics.py, but I can't guarantee from reading the code alone what it will actually be when used.

To reproduce this, should I just try to reproduce the study https://github.com/leeping/forcebalance/tree/master/studies/024_openff_evaluator ?

jthorton commented 2 years ago

Yeah I think it should be except in the case where its None for gas calculations so the following will always cause an error

from openff.units.openmm import to_openmm
pressure = to_openmm(None)

I think this is basically what is done here.

Yes that test should cause the error.

jthorton commented 2 years ago

Also not sure what happens here and here it feels like it expects to_openmm to return None if you give it None.

mattwthompson commented 2 years ago

I'm looking at this again with fresh eyes and I think #463 manually handling the case of None pressure is an improvement, but I wonder if should also be dealt with in the units repo. This

>>> from openff.units.openmm import from_openmm
>>> from_openmm(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mattthompson/miniconda3/envs/openff-evaluator-test/lib/python3.9/site-packages/openff/utilities/utilities.py", line 75, in wrapper
    return function(*args, **kwargs)
  File "/Users/mattthompson/miniconda3/envs/openff-evaluator-test/lib/python3.9/site-packages/openff/units/openmm.py", line 121, in from_openmm
    openmm_unit_ = openmm_quantity.unit
AttributeError: 'NoneType' object has no attribute 'unit'

is confusing and arguably undefined. - if I wasn't familiar with this codebase I'd have no idea where to start debugging this. If not making from_openmm(None) return None I think there should be an exception raised on the basis that the input it not any sort of quantity object.