Fix NoJump transformation error when applied outside the first frame … #4918
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
|
@TejasNangru thank you for your PR. Developers will review but please aware that this may take some time because everybody is quite busy and nobody gets paid to review. The first thing you should make sure is that the tests all pass, i.e., there should be no failing checks. If you have specific questions then please ask. |
|
As of now, I dont have any specific question. |
Before a proper review, the tests need to pass. They likely need to be modified, since you changed the warning. See here, for example. |
|
I don't know why still some of tests are failing. |
|
Thanks sir for the review, I will do the required changes. |
|
@IAlibay Please re-review it. |
| @@ -322,8 +322,8 @@ def test_nojump_constantvel_stride_2(nojump_universes_fromfile): | |||
| """ | |||
| Test if the nojump transform warning is emitted. | |||
| """ | |||
| match = "Currently jumping between frames with a step of more than 1." | |||
| with pytest.warns(UserWarning, match=match): | |||
| match = "NoJump transformation must be applied from the first frame onward." | |||
There was a problem hiding this comment.
please don't overwrite the existing test, keep the old test and add a new one that tests specifically the error being raised
| match = "Currently jumping between frames with a step of more than 1." | ||
| with pytest.warns(UserWarning, match=match): | ||
| match = "NoJump transformation must be applied from the first frame onward." | ||
| with pytest.raises(ValueError, match=match): | ||
| u = nojump_universes_fromfile | ||
| for ts in u.trajectory[::2]: # Exercises the warning. |
There was a problem hiding this comment.
This doesn't trigger the ValueError because it starts from 0 and skips by 2. Please write a separate test for the ValueError failure
There was a problem hiding this comment.
def test_nojump_raises_valueerror_outside_first_frame(nojump_universes_fromfile):
"""
Test if NoJump transformation raises ValueError when applied outside the first frame.
"""
match = "NoJump transformation must be applied from the first frame onward."
with pytest.raises(ValueError, match=match):
u = nojump_universes_fromfile
u.trajectory[-1] # Move to the last frame
u.trajectory.add_transformations(NoJump()) # Add NoJump transformation
next(u.trajectory) # Trigger the transformation by iteratingSir, It works fine? @IAlibay
|
I unfortunately don't have time to do more reviews, hopefully this is enough to get you on the path towards a merge. Can anyone from @MDAnalysis/coredevs take over here? |
As you can see, tests are still failing (which automatically implies the PR needs indeed more modifications). Please have a look at the test failures. Feel free to ask specific questions should you need to. |
It's my first time working with Github tests, can you please explain briefly what i have to do for each test which fails? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4918 +/- ##
===========================================
- Coverage 93.66% 93.63% -0.04%
===========================================
Files 177 189 +12
Lines 21850 22918 +1068
Branches 3079 3080 +1
===========================================
+ Hits 20466 21459 +993
- Misses 933 1007 +74
- Partials 451 452 +1 ☔ View full report in Codecov by Sentry. |
|
@RMeli @orbeckst I was on 6 successful tests earlier, now I did some changes and on 21 successful tests, |
|
Please push your latest changes. The codecov patch test failure with 0% coverage is a bit odd, I'll have a look. |
| @@ -328,6 +328,17 @@ def test_nojump_constantvel_stride_2(nojump_universes_fromfile): | |||
| for ts in u.trajectory[::2]: # Exercises the warning. | |||
| pass | |||
|
|
|||
| def test_nojump_raises_valueerror_outside_first_frame(nojump_universes_fromfile): | |||
There was a problem hiding this comment.
Good that you wrote a separate test. However, this test does not trigger the failure that you're testing for and that's why code coverage is 0%.
You have to write code here that generates the failure case.
You also must match the error message for the failure ("NoJump transformation must be applied from the first frame onward.").
There was a problem hiding this comment.
@TejasNangru this test you wrote was nearly correct, but you should use the nojump_universe fixture rather than nojump_universes_fromfile . nojump_universes_fromfile is already applying the transformation, so an error is raised when you try to apply the transformation again in your test
|
|
||
| # Ensure NoJump is applied from the first frame onwards | ||
| if ts.frame > 0 and self.prev is None: | ||
| raise ValueError( |
There was a problem hiding this comment.
From the codecov comment below you can see that the raise ValueError is never triggered by the tests. Thus, you have to improve your test.
|
@p-j-smith I did changes you told, but still codecov is getting failed. |
Fixes #4915
This PR fixes issue #4915, where the NoJump transformation failed with an unintuitive TypeError when applied outside the first frame.
Changes made in this Pull Request:
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--4918.org.readthedocs.build/en/4918/