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 a writer_kwargs argument to MDAReporter and speed up testing #7

Closed
wants to merge 8 commits into from

Conversation

orionarcher
Copy link
Contributor

@orionarcher orionarcher commented Mar 31, 2024

Fixes #6

Changes made in this Pull Request:

  • format code with Black
  • 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 Add a writer_kwargs argument to MDAReporter. Add a writer_kwargs argument to MDAReporter and cleanup testing Mar 31, 2024
@orionarcher orionarcher changed the title Add a writer_kwargs argument to MDAReporter and cleanup testing Add a writer_kwargs argument to MDAReporter and speed up testing Mar 31, 2024
@orionarcher
Copy link
Contributor Author

Would love a look from @richardjgowers or @sef43 when y'all have time! I think the changes are pretty innocuous and would help me solve a downstream use case.

@sef43
Copy link
Owner

sef43 commented Apr 2, 2024

Thanks for the contribution!, can this please be re-done without the formatting changes please? The formating change should be a seperate PR.

@codecov-commenter
Copy link

Codecov Report

Merging #7 (f2ecd4f) into main (d665b3d) will decrease coverage by 31.09%.
The diff coverage is 37.83%.

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

Additional details and impacted files

@orionarcher
Copy link
Contributor Author

orionarcher commented Apr 2, 2024

Are you opened to the formatting changes? I opened a PR #8 with just the Black formatting.

If possible, I'd like to merge that and then reopen this PR so I don't have to sort out the logic from formatting changes. (A little lazy I admit.)

@sef43
Copy link
Owner

sef43 commented Apr 2, 2024

Currently yes I am against the formatting changes to any of the existing files. The current formatting follows the openmm formatting for Reporter classes. There is no reason to deviate from this.

I don't have to sort out the logic from formatting changes.

As you point out here the key issue is that the unnecessary formatting changes obscure the functional changes to the code. I think MDAnalysis does not allow black formatting of existing code, only new code, for this reason.
MDAnalysis/mdanalysis#2450

@orionarcher
Copy link
Contributor Author

Fair enough, I understand the desire to avoid black and stay compatible with upstream formatting.

I added a new PR #9 that implements the testing and kwargs changes without the black formatting.

@orionarcher orionarcher closed this Apr 2, 2024
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