Adding smaller test data #55
Conversation
|
Hello @BFedder! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-08-02 10:23:10 UTC |
Codecov ReportBase: 87.68% // Head: 82.97% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #55 +/- ##
==========================================
- Coverage 87.68% 82.97% -4.72%
==========================================
Files 5 3 -2
Lines 406 276 -130
==========================================
- Hits 356 229 -127
+ Misses 50 47 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Okay, so coverage of panedr.py and pyedr.py appears to be unaffected. Should we replace the test data files then, or am I missing a reason why that would be a bad idea? |
|
Not in my mind as long as you clarify how the files are different (both here and possible in a README also?) |
|
@hmacdope - It all depends on how small the package gets with it, if the package is still > a couple of MB when inflated then it's not worth it. The whole point of pyedr was that pandas was too heavy a dependency. We're breaking that idea if we just end up adding more tests. If we don't package the tests then reducing the test sizes don't matter (in fact you just end up using more git history space). |
|
The new files are from a very short simulation of lysozyme that I ran just to get them. If we do go ahead and replace cat.edr and cat.xvg with the new files, then this would reduce the test data size from 29 MB to 0.5 MB. As far as I can tell, there is no practical difference, as the coverage of panedr.py and pyedr.py is unchanged. The new files do only contain 4 frames (down from 14100), however, so I am not sure how true of a test of output verbosity this still poses. |
That's reasonably small tbh, @jbarnoud would be the right judge of whether or not this is a suitable replacement for this test. |
|
I'm on my phone so it's a bit difficult to do better than work from memory. However, I think I remember that the point of the test file is to make sure we concatenate edr files the same way as gromacs. I don't see in the changes how it would have tested that, though 🤔 What I see, however is that the progress bar does not get tested as much as it was. Because it adapts to the number of frames, there should be a replacement test somehow to cover the feature. |
|
Coverage also analyzed the test files. They should be excluded from coverage reporting. |
Would it be worthwhile replacing the progress output with |
|
I have tried changing pyedr now so that the progress bar is provided by tqdm instead. This also allowed me to remove the tests for the progress bar. However, I can't get codecov to work properly now... Please could someone help? |
Is this the upload failure or is there something else going wrong here? |
panedr/requirements.txt
Outdated
| @@ -1,3 +1,4 @@ | |||
| pandas | |||
| pyedr | |||
| pbr | |||
| tqdm | |||
There was a problem hiding this comment.
pyedr/pyedr/pyedr.py
Outdated
| end = time.time() | ||
| if verbose: | ||
| print('\rLast Frame read : {}, time : {} ps' | ||
| .format(ifr, frame.t), | ||
| end='', file=sys.stderr) | ||
| print('\n{} frame read in {:.2f} seconds'.format(ifr, end - begin), | ||
| print('\n{} frames read in {:.2f} seconds'.format(ifr, end - begin), |
There was a problem hiding this comment.
Can we just get rid of timing altogether? You get iterations per second out of the standard tqdm output, otherwise if we really want total time, we could do something like bar_format="{l_bar}{bar}| {n_fmt}/{total_fmt} [{elapsed}]" or some variation of the tqdm barformat parameter?
There was a problem hiding this comment.
I think there isn't really a need for timing as a default or even verbose option. IMO TQDM has enough timing info.
There was a problem hiding this comment.
I guess you'll still need the verbose flag to turn tqdm on/off?
| @@ -28,6 +28,7 @@ python_requires = >= 3.6 | |||
| install_requires = | |||
| numpy | |||
| pbr | |||
| tqdm | |||
There was a problem hiding this comment.
Forgot to also say, can you add tqdm to the gh actions CI yaml file?
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
|
Done! |
I'm for now just using this PR to trigger codecov, as this is the fastest way I know to determine coverage.
To make the pyedr test files smaller, I added the edr test file I created for the EDRReader and changed the tests for pyedr accordingly. The tests now pass, but I want to make sure that the much smaller file still triggers all of the same code to be tested.