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

Add writer_kwargs to MDAReporter and speed up testing #9

Merged
merged 9 commits into from
Apr 5, 2024

Conversation

orionarcher
Copy link
Contributor

Fixes #6

Changes made in this Pull Request:

  • added a writer_kwargs argument to MDAReporter that is passed to mda.Writer
  • migrate testing to use serialized simulation instead of re-instantiating each time. Much faster, all tests pass locally.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@orionarcher orionarcher changed the title New kwargs testing Add writer_kwargs to MDAReporter and speed up testing Apr 2, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Merging #9 (acae341) into main (d665b3d) will decrease coverage by 4.42%.
The diff coverage is 88.46%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@sef43
Copy link
Owner

sef43 commented Apr 3, 2024

this is looking good.

Does this now work with the MDAnalysis fix in #5 ?

Can we add a test that writes positions to a h5 and checks it is correct by reading it back in with Mdanalysis?

Additionally, can we have a test that writes velocities and forces to the h5 and checks they are being reported correctly? this might need to be done after #4 in which case we can just add a test that h5 positions are correctly reported and the velocities can be done on a separate PR. Perhaps a warning can be added for now if H5 is used without specifying that velocities should not be written.

@orionarcher
Copy link
Contributor Author

orionarcher commented Apr 3, 2024

Does this now work with the MDAnalysis fix in #5 ?

Yes, the code is working locally and should work with the next release of MDA.

Can we add a test that writes positions to a h5 and checks it is correct by reading it back in with Mdanalysis?

I think this would just require adding "H5MD" here. That said, because of the upstream MDA error, the test would currently fail.

Maybe we could save tests on "H5MD" for a later PR after the next MDA release?

Perhaps a warning can be added for now if H5 is used without specifying that velocities should not be written.

To be clear, velocities aren't causing the error, it's the kinetic energy and potential energy outputs, which can be turned off with writer_kwargs. To write any output to H5MD I think one would need to specify the writer_kwargs as {"potential_energy": False, "kinetic_energy": False} which would avoid the error discussed in MDA PR #4548. I could add some code that would automatically add that to the writer_kwargs if the file format is "H5MD", and we could take it out after the next MDA release. edit: nvm, no way to stop it from failing.

@orionarcher
Copy link
Contributor Author

Added a fix for issue #4.

I think this is ready to merge.

Since writing H5 is broken upstream, I don't think it's worth writing tests yet.

@sef43 sef43 self-requested a review April 5, 2024 08:09
@sef43 sef43 merged commit d2d1b1d into sef43:main Apr 5, 2024
14 checks passed
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.

MDAReporter, when reporting H5MD, has no way to leave the forces or velocities out
3 participants