Fix start years #123
Conversation
… ERA5-Land test for inaccecibility of the years prior to 1981
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## back_extension_update #123 +/- ##
========================================================
Coverage ? 98.94%
========================================================
Files ? 6
Lines ? 379
Branches ? 0
========================================================
Hits ? 375
Misses ? 4
Partials ? 0 Continue to review full report at Codecov.
|
bvreede
left a comment
bvreede
left a comment
There was a problem hiding this comment.
Thanks so much @malininae! Indeed, this is a very necessary update, so we are very happy with your contribution.
There is a couple changes I'm requesting; small, and they should be largely as suggested commits, so I hope it is not too much work for you.
First off, we are very happy to have you recognized as a contributor! 🎉
The new version release, however, I'd like to do as a separate PR, and it would be good to focus this PR on the change in startyear. So, I'm suggesting to revert some version changes you proposed in this PR.
I've also changed the base of the PR, so we can merge this to a branch that allows us to add a few more changes before we merge it to main.
Thanks again for your contribution!
Peter9192
left a comment
Peter9192
left a comment
There was a problem hiding this comment.
Thanks for the PR! We'd have to make a new release for the changes to be available through pip and conda. Also we might want to think about deprecating support for the preliminary back extension, but that's for later.
@malininae are you OK with us taking over from here and getting it ready and released?
| # no land available for back extension | ||
| argv = ['monthly', '--startyear', '1980', '--endyear', '1980', | ||
| '--variables', 'total_precipitation', '--synoptic', | ||
| '--ensemble', '--land'] | ||
| args = cli._parse_args(argv) | ||
| with pytest.raises(AssertionError): | ||
| cli._execute(args) | ||
|
|
There was a problem hiding this comment.
While this test was a bit ill-described, I do think it is important to keep it, in a slightly modified form. We need to raise an error if the --land and --prelimbe flags are used together, as there is no back extension for era5-land.
There was a problem hiding this comment.
@Peter9192, thanks for the suggestion, I looked into it. So, if I understand correctly this particular test was more about --land being incompatible with the years prior to 1981, not back extension as comment says. The incompatibility of --prelimbe and --land are tested in test_fetch.py, because it is the place that gives you the ValueError, if those two tags are used together. As highlighted in #130, the request with --ensemble and --land actually goes through but fails on the cdsapi end. My suggestion is, for this pull request delete this test, and I will open a new PR with adding the AssertionError in case --ensemble and --land are used together. Then, I will re-add this test, but testing the --ensemble and --land. What do you think? @bvreede what's your opinion?
There was a problem hiding this comment.
Right! So I did notice that there was a discrepancy between the description (land and back extension not compatible) and the actual test (land not available for years prior to 1981). But you're right, I overlooked that there's also the issue of --land and --ensemble being incompatible. Your suggested approach sounds good to me 👍
There was a problem hiding this comment.
Thanks for figuring this out; indeed, that's a good idea, let's do it!
|
@bvreede @Peter9192 Thanks! I will clean it up next week if you don't mind, it's pretty straight forward, so I'm good from here! |
Co-authored-by: Barbara Vreede <b.vreede@esciencecenter.nl>
Co-authored-by: Barbara Vreede <b.vreede@esciencecenter.nl>
Co-authored-by: Barbara Vreede <b.vreede@esciencecenter.nl>
Co-authored-by: Barbara Vreede <b.vreede@esciencecenter.nl>
Co-authored-by: Barbara Vreede <b.vreede@esciencecenter.nl>
bvreede
left a comment
bvreede
left a comment
There was a problem hiding this comment.
Thanks @malininae! My apologies for the delay on reviewing this; it got snowed under after I got back from holiday.
I'm happy to merge this now; let's also fix the issue you raised in #130 and make a new release!
| # no land available for back extension | ||
| argv = ['monthly', '--startyear', '1980', '--endyear', '1980', | ||
| '--variables', 'total_precipitation', '--synoptic', | ||
| '--ensemble', '--land'] | ||
| args = cli._parse_args(argv) | ||
| with pytest.raises(AssertionError): | ||
| cli._execute(args) | ||
|
|
There was a problem hiding this comment.
Thanks for figuring this out; indeed, that's a good idea, let's do it!
3 participants
Hi all,
Here is a pull request to change the start years for ERA5 to 1959 and ERA5-Land to 1950 for both. I checked and they both work. The test, which returns assertation error for ERA5-Land being outside the bounds was removed. This pull request resolves issue #122.
I was thinking of adding a warning, in case one wants to download a preliminary version from 1959 to 1978, but by the looks of it, era5cli doesn't use warnings package. Let me know if I should add the warning in some fashion, or just leave it as it is.
I also humbly added my name to the creators list, but if you think the contribution is too small, feel free to remove it. Please, let me know if anything else needs to be done.
Cheers