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

Restore velocities only if found in serialized nc file #613

Merged
merged 7 commits into from Aug 2, 2022

Conversation

ijpulidos
Copy link
Contributor

@ijpulidos ijpulidos commented Jul 28, 2022

Description

Restores velocities only if variable is found in the serialized files. This allows back compatibility with serialized files using openmmtools<0.21.3, since velocities were added in that release.

Resolves #612

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog

Bug when restoring serialized files generated by ``openmmtools<0.21.3``. Fixed by catching the ``KeyError`` exception when deserializing.

@ijpulidos ijpulidos added this to the 0.21.5 milestone Jul 28, 2022
@ijpulidos
Copy link
Contributor Author

ijpulidos commented Jul 29, 2022

I had to add .nc data files using an older version of openmmtools (0.21.2) to test that we can resume simulations from them correctly.

I thought about emulating this behavior by removing the velocities variables from the NetCDF Datasets themselves, but NetCDF doesn't have an API for removing variables.

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a note that this fix enables reading of older nc files? Other than that, LGTM

@ijpulidos
Copy link
Contributor Author

ijpulidos commented Aug 2, 2022

Please note that I had to restructure the tests a bit, you can see the same number of tests are getting run (just in case you are wondering). But I had to make a new base class for avoiding running redundant tests in child classes.

The core reason for this is that the serialized objects in the data .nc files that the new test are using, are meant to be used only with the MultiStateSampler object and not all the others that inherit from it in the tests, due to missing variables/fields in the files.

@mikemhenry @jchodera This should be good to go now, can you please review it? Thanks.

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhang-ivy zhang-ivy merged commit 85f6220 into main Aug 2, 2022
@zhang-ivy zhang-ivy deleted the 612-serialized-velocities-fix branch August 2, 2022 18:51
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

Successfully merging this pull request may close these issues.

Load serialized files generated with openmmtools<=0.21.2
3 participants