-
Notifications
You must be signed in to change notification settings - Fork 44
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
Test interp nominal lon #365
Test interp nominal lon #365
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting in more work into polishing this up @JoranAngevaare! I would much prefer to keep this in pure pytest, but I might also be missing something here.
tests/test_preprocessing.py
Outdated
def test_new_works(self): | ||
lons = self._get_dummy_longitude() | ||
lons_parsed = _interp_nominal_lon(lons) | ||
if not self._lons_parsed_make_sense(lons, lons_parsed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure this will actually test correctly with pytest. I have only ever written funtions with assert statements. See https://docs.pytest.org/en/stable/getting-started.html
If you want to ensure that a certain Exceptio is raised you can wrap the statement with
with pytest.raises(...)
...
tests/test_preprocessing.py
Outdated
lons_parsed = self._interp_nominal_lon_old(lons) | ||
if self._lons_parsed_make_sense(lons, lons_parsed): | ||
raise ValueError( | ||
"Parsed lons should be gibberish, but somehow the old implementation also works?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment. That will hopefully lead this to actually fail? Once that is the case I wouöd suggest to remove the "old way" logic to keep this test more compact
@jbusecke of course value-errors result in failed tests and this syntax does trigger the tests:
But sure, let me write assert statements if that's more conform the way things are done for xmip |
@jbusecke as requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for iterating on this @JoranAngevaare . Really appreciate the ongoing work. I had a minor ? about s comment, but otherwise I think this is ready!
tests/test_preprocessing.py
Outdated
lons_parsed = _interp_nominal_lon(lons) | ||
assert _lons_parsed_make_sense( | ||
lons, lons_parsed | ||
), "Parsed lons after the fix of #296 are still bad?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are now passing, so this comment confuses me slightly. Is it a leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the message that the AssertionError
will produce once failed, see for example the message in https://github.com/jbusecke/xMIP/actions/runs/9857864431/job/27217936984 (pytest also prints the lons
, and lons_parsed
so it looks a bit messy).
Of course, it shouldn't (and doesn't) fail so I could as well remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be awesome! Ill merge it right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I already pushed the commit
Thanks for verifying this. And it obviously makes sense (Ill keep that in mind for future test designs, and thanks for teachibg me this!). I should have tested this myself (but am using gh on mobile so am a bit restricted here). |
Fix test for
_interp_nominal_lon
This PR is some leftover work from #296 (and #300)
replace_x_y_nominal_lat_lon
does not work for > 360lon
coordinates #295._interp_nominal_lon
#296PR checks
replace_x_y_nominal_lat_lon
does not work for > 360lon
coordinates #295, Adding new contributors to CITATION.cff #358pre-commit run --all-files
whats-new.rst
api.rst