add functionality for download of subregion #70
Conversation
|
Things to fix and add:
|
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
==========================================
+ Coverage 98.20% 98.31% +0.11%
==========================================
Files 7 7
Lines 335 357 +22
==========================================
+ Hits 329 351 +22
Misses 6 6
Continue to review full report at Codecov.
|
…want-python-argparse-to-throw-an-exception-rather-than-usage
Co-authored-by: Peter Kalverla <[email protected]>
|
Some things to pay attention to when reviewing:
|
era5cli/fetch.py
Outdated
| and -180 <= W <= 180 | ||
| and -180 <= E <= 180 | ||
| and N > S | ||
| and W != E |
There was a problem hiding this comment.
I'm surprised as I'd also expect W < E, but indeed the online CDS checker just accepts that. Does that lead to unexpected filenames?
And maybe we can issue a warning in this case (instead of an error)? Something like: "Be aware, this will probably work but we recommend changing it to avoid confusion"?
There was a problem hiding this comment.
I assumed that W-E was different, because they are the western and eastern coordinates of a grid that wraps around the world. You could e.g. have a grid from 120W to -120E to get the pacific ocean, or from -120W to 120E to get most continents.
The filenames are probably confusing if you would read them as degrees on the map, which is what W-E-N-S may do, rather than numeric coordinates of the globe, with the W/E/N/S indicating the borders of the map you have chosen.
I can't even express this properly here, let alone in a filename :/ really not sure what is the best approach...
There was a problem hiding this comment.
Perhaps instead of W/E/N/S we could work with Left/Right/Top/Down, as that is the information we need. [edit: or use the ymax xmin ymin xmax terminology, which is already in the cli, and remove all references to WENS as this is simply confusing]
Co-authored-by: Peter Kalverla <[email protected]>
Co-authored-by: Peter Kalverla <[email protected]>
Co-authored-by: Peter Kalverla <[email protected]>
Co-authored-by: Peter Kalverla <[email protected]>
era5cli/fetch.py
Outdated
| coords = [str(int(c)) for c in self.area] | ||
| fname += '_[' + ']['.join(coords) + ']' |
There was a problem hiding this comment.
Beware that round(1.5) = 2 while int(1.5)=1.
There was a problem hiding this comment.
What about something like:
lon = lambda x: f"{x}E" if x>0 else f"{abs(x)}W"
lat = lambda y: f"{y}N" if y>0 else f"{abs(x)}S"
xmin, xmax = -10, 10
ymin, ymax = 45, 55
name = f"{lon(xmin)}-{lon(xmax)}_{lat(ymin)}-{lat(ymax)}"
--> '10W-10E_45N-55N'There was a problem hiding this comment.
Thanks muchly! Incorporated.
Questions (to pay attention to in the new commit):
- I added that it does not add NS/EW notation if the coordinate is 0. Is this correct? [added a test for this as well; line 223 in test_fetch.py gives an example].
- Are they in the right order like this?
I liked having a direct relationship between the user input and the file name; that is the only thing I'm sad to lose...
There was a problem hiding this comment.
Nicely done 👍 . I think I'd prefer 0N and 0E over just plain zeros.
|
Thanks @bvreede and @aytacpacal for adding this awesome feature! |
This draft pull request is a sequel to #66,
and preemptively includes #67, so should not be considered until after the latter is merged.The work was started by @aytacpacal and aims to allow users to download data from a subregion of the globe by adding coordinates to the command.
Requested changes from #66 have been included, and tests have been added.
Some tests in test_cli.py have been set to xfail as I am unsure what error message to use; help would be appreciated!Closes #44.