Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and consistent handling of OpenMMState fields #117

Open
5 tasks
salotz opened this issue Nov 8, 2023 · 2 comments
Open
5 tasks

Refactor and consistent handling of OpenMMState fields #117

salotz opened this issue Nov 8, 2023 · 2 comments

Comments

@salotz
Copy link
Collaborator

salotz commented Nov 8, 2023

Currently the mechanisms for wrapping and retrieving dynamically available fields in the OpenMMState and how it wraps the OpenMM.State is very flaky and causes problems for the tail of attributes that aren't actually ever used in practice very much (e.g. parameter derivatives).

I would like to refactor this so that these only cause problems when a user actually wants these things.

A checklist:

  • refactor the code to be easier to maintain by using template functions for most functionality
  • add more consistency checks requiring certain data ahead of when it is needed
  • allow for requesting only certain fields for the dict method
  • support querying what fields are actually available more easily
  • add better and more consistent error handling and remove unnecessary warnings (see also Proper error handling for missing OpenMM state variables #102 )

I have some work towards this already and will need to just finish some things on this checklist to maintain full support and backwards compatibility.

@alexrd
Copy link
Collaborator

alexrd commented Nov 13, 2023

Adding some urgency to this, as it seems that the latest openmm will raise an exception when we call getEnergyParameterDerivatives on a state that doesn't have them:

Traceback (most recent call last):
...
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/sim_manager.py", line 492, in _run_cycle
    reporter.report(**report)
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/reporter/hdf5.py", line 523, in report
    walker_data = walker.state.dict()
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/runners/openmm.py", line 1194, in dict
    for key, value in self.omm_state_dict().items():
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/runners/openmm.py", line 1179, in omm_state_dict
    param_derivs = self.parameter_derivatives_features()
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/runners/openmm.py", line 1155, in parameter_derivatives_features
    parameter_derivatives = self.parameter_derivatives_values()
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/runners/openmm.py", line 1047, in parameter_derivatives_values
    if self.parameter_derivatives is None:
  File "/mnt/ufs18/home-126/alexrd/wepy/src/wepy/runners/openmm.py", line 1020, in parameter_derivatives
    return self.sim_state.getEnergyParameterDerivatives()
  File "/mnt/home/alexrd/anaconda3/envs/wepy/lib/python3.10/site-packages/openmm/openmm.py", line 4965, in getEnergyParameterDerivatives
    return _openmm.State_getEnergyParameterDerivatives(self)
openmm.OpenMMException: Invoked getEnergyParameterDerivatives() on a State which does not contain parameter derivatives.

@salotz
Copy link
Collaborator Author

salotz commented Nov 14, 2023

Yes this is the error I was finding as well. Interestingly it doesn't show up when using Reference or CPU platform.

I'll try to merge a comprehensive fix of this very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants