Conversation
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay , I know this is still [WIP] , so just some preliminary comments, but otherwise lgtm!
Co-authored-by: Hannah Baumann <43765638+hannahbaumann@users.noreply.github.com>
Co-authored-by: Hannah Baumann <43765638+hannahbaumann@users.noreply.github.com>
|
@hannahbaumann if you could take over here, especially with the tests that would be grand! The figshare with the new files is here: https://figshare.com/articles/dataset/OpenFE_1_3_0beta_example_results_files/28263203 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
===========================================
+ Coverage 60.06% 72.10% +12.03%
===========================================
Files 6 7 +1
Lines 298 337 +39
===========================================
+ Hits 179 243 +64
+ Misses 119 94 -25 ☔ View full report in Codecov by Sentry. |
|
The tests are quite flaky for some reason, I wonder if it's related to the size of the original NetCDF file. I added a |
|
@IAlibay : I can't request a review from you since you had opened this PR (sorry, I should have made changes in a separate branch instead), could you review the small changes I made? I added the files for the skipped frames in the conftest.py and modified/added the test to check the positions and velocities. |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Looks good @hannahbaumann - couple of things and then I think it's good to go.
| assert u.trajectory.n_frames == 6 | ||
| for ts in u.trajectory: | ||
| assert np.all(u.atoms.positions > 0) | ||
| with pytest.raises(mda.exceptions.NoDataError, match='This Timestep has no velocities'): |
There was a problem hiding this comment.
Possibly do this outside of the loop? As far as I remember, MDAnallysis doesn't support variable velocities, so if it fails on one timestep it'll fail on all timesteps.
| range(0, dataset.dimensions['iteration'].size, dataset.PositionInterval) | ||
| ] | ||
| else: | ||
| wmsg = ('This is an older NetCDF file that does not yet contain ' |
There was a problem hiding this comment.
In the tests using the old file, could you wrap the universe creation with a pytest.warns that checks for this please?
| will return ``None``. | ||
| """ | ||
| # Case: no box_vectors were stored at this frame | ||
| if dataset.variables['box_vectors'][frame_num][replica_index].mask.all(): |
There was a problem hiding this comment.
Could you add a test in test_multistate with the skipped netcdf that tries to access the positions and unitcell at frame 1 and gets a None?
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
This looks good to me, thanks @hannahbaumann !
hannahbaumann
changed the title
2 participants
Fixes #41
TODO: