Download subregion #66

aytacpacal · 2021-01-12T00:23:41Z

Added ability to download subregion

closes #44

Peter9192 · 2021-01-12T11:41:28Z

Hey @aytacpacal welcome and cheers for opening this PR! I added a link to a corresponding issue (#44).

Peter9192

Hi @aytacpacal ,
Thank you very much for contribution this PR to era5cli! This is much appreciated.

I have left a couple of comments and suggestions, to improve the code but mostly to make sure it is well documented. Additionally, I have the following suggestions:

It would be good to add a check in the cli.py file and raise an error if invalid coordinates are passed.
It would be good to also add a test for this new argument (see tests/test_cli.py)
Please add your details to the citation.cff and .zenodo.json files if you would like to be recognized for your contributions.

Let me know if you need help with any of these recommendations. I'm happy to help.

Peter9192 · 2021-01-15T14:32:21Z

era5cli/cli.py

@@ -144,6 +144,20 @@ def _build_parser():
                             ''')
    )

+    rgn = argparse.ArgumentParser(add_help=False)


Do we need a new parser? Why not just add the area argument to common?

Peter9192 · 2021-01-15T14:37:57Z

era5cli/cli.py

+                            Coordinates to download data for. Defaults to whole
+                            available region. For subregion extraction, provide coordinates 
+                            as {ymax,xmin,ymin,xmax}


Maybe this provides some extra help as to how the list can be specified on the command line:

Suggested change

Coordinates to download data for. Defaults to whole

available region. For subregion extraction, provide coordinates

as {ymax,xmin,ymin,xmax}

Use this argument to specify a subregion for which to

download the data. Coordinates should be provided as

a space separated list as {ymax, xmin, ymin, xmax}.

Defaults to `--area 90 -180 -90 180`, i.e. the whole globe.

Peter9192 · 2021-01-15T14:50:43Z

era5cli/cli.py

@@ -187,7 +201,7 @@ def _build_parser():
    )

    hourly = subparsers.add_parser(
-        'hourly', parents=[common, mnth, day, hour],
+        'hourly', parents=[common, rgn, mnth, day, hour],


This is not necessary if we just add it to the common group

Peter9192 · 2021-01-15T14:50:53Z

era5cli/cli.py

@@ -211,7 +225,7 @@ def _build_parser():
    )

    monthly = subparsers.add_parser(
-        'monthly', parents=[common, mnth],
+        'monthly', parents=[common, rgn, mnth],


Peter9192 · 2021-01-15T14:57:45Z

era5cli/fetch.py

+        area: list(float)
+            Lon-lat values to download data for. (-180 and 180, -90 and 90)


Let's try to be very clear about how to represent the range, as I think this order can be quite confusing.

Suggested change

area: list(float)

Lon-lat values to download data for. (-180 and 180, -90 and 90)

area: list(float)

Coordinates to extract a subregion. Specified as {ymax, xmin, ymin, xmax}

with x and y in the range -180, +180 and -90, +90, respectively.

Peter9192 · 2021-01-15T14:58:58Z

era5cli/fetch.py

@@ -84,6 +86,8 @@ def __init__(self, years: list, months: list, days: list,
        """list(int): List of pressure levels."""
        self.variables = variables
        """list(str): List of variables."""
+        self.area = area
+        """list(int): List of coordinates"""


In the argparse options I think this was a float, no?

Suggested change

"""list(int): List of coordinates"""

"""list(float): Coordinates specifying the subregion that will be extracted."""

Peter9192 · 2021-01-15T15:04:57Z

era5cli/utils.py

@@ -186,4 +186,4 @@ def _append_netcdf_history(ncfile: str, appendtxt: str):
            {}'''.format(appendtxt, ncfile.history))
    except AttributeError:
        ncfile.history = appendtxt
-    ncfile.close()
+    ncfile.close()


https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file

Please use flake8 to see whether the code adheres to the PEP8 python style guidelines.

Peter9192 · 2021-01-25T13:28:38Z

Hello @aytacpacal ,
Thanks again for opening a pull request. I was wondering if you have seen my review, and whether you are planning to continue with this. Can you give an update?

aytacpacal · 2021-01-25T13:36:21Z

Hi @Peter9192, yes, thanks for your comments. Unfortunately, I did not have time to look. I plan to continue but do not know when it will be :(

Peter9192 · 2021-01-25T14:03:25Z

Hi @Peter9192, yes, thanks for your comments. Unfortunately, I did not have time to look. I plan to continue but do not know when it will be :(

Okay thanks for the reply. It's good to know this is still on your radar. Cheers!

Peter9192 · 2021-02-16T16:19:47Z

Hello again @aytacpacal how are you doing? Can you give another update about your plans regarding this PR? If you don't have the time, we might be able to help out and finish the work on this one. Let me know what option you prefer :-)

aytacpacal · 2021-02-16T16:30:29Z

Hi @Peter9192, yeah I am struggling with my time management, so it will be better if someone else takes a look :(

bvreede · 2021-02-18T11:24:52Z

Hi @aytacpacal thanks again so much for opening this PR! 🎉 With your permission, I'll continue work on implementing the subregion download.

There are a few ways we can do this: as you may have noticed, I've copied your subregion branch to this repository, as I didn't have access to your forked repo. I am currently working on this branch.

Option 1: I can open a new PR with this branch.
Option 2: I can open a PR to your forked repository, which when it's merged would automatically update this current PR.

Working with a new PR (option 1) will be easier for me and wouldn't need your continuous attention, but while you would remain as the commit author for the commits already made on the branch, you wouldn't be the author of the PR itself. Should you want to add anything to the PR, option 2 is better, as you have no direct access to the branch on this repository.

Could you let me know which option you prefer?

Regarding recognition for your work: if you want, we can add your details to the citation.cff and .zenodo.json files, and I'm fairly certain you would be listed as a contributor on the github page as a commit author. And of course you will have our eternal gratitude ;)

Thanks again!

aytacpacal · 2021-02-18T13:06:18Z

Hi @bvreede, you can use whichever is easier for you :) I thank you for creating this software, your eternal gratitude is enough for me 🤗

bvreede · 2021-03-04T10:07:32Z

Hi @aytacpacal just to let you know, @Peter9192 just closed #70, and subregion support has been added! And you're now an official contributor to this repository :) Thanks much, all the best!

aytacpacal added 3 commits January 12, 2021 00:47

Subregion selection

eb10afe

Modified README

352404f

Modified README

5ef8069

Peter9192 requested changes Jan 15, 2021

View reviewed changes

bvreede mentioned this pull request Feb 18, 2021

add functionality for download of subregion #70

Merged

bvreede closed this Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download subregion #66

Download subregion #66

aytacpacal commented Jan 12, 2021 •

edited by Peter9192

Loading

Peter9192 commented Jan 12, 2021

Peter9192 left a comment

Peter9192 Jan 15, 2021

Peter9192 Jan 15, 2021

Peter9192 Jan 15, 2021

Peter9192 Jan 15, 2021

Peter9192 Jan 15, 2021

Peter9192 Jan 15, 2021

Peter9192 Jan 15, 2021

Peter9192 commented Jan 25, 2021

aytacpacal commented Jan 25, 2021

Peter9192 commented Jan 25, 2021

Peter9192 commented Feb 16, 2021

aytacpacal commented Feb 16, 2021

bvreede commented Feb 18, 2021

aytacpacal commented Feb 18, 2021

bvreede commented Mar 4, 2021

-                            Coordinates to download data for. Defaults to whole
-                            available region. For subregion extraction, provide coordinates
-                            as {ymax,xmin,ymin,xmax}
+                            Use this argument to specify a subregion for which to
+                            download the data. Coordinates should be provided as
+                            a space separated list as {ymax, xmin, ymin, xmax}.
+                            Defaults to `--area 90 -180 -90 180`,  i.e. the whole globe.

		area: list(float)
		Lon-lat values to download data for. (-180 and 180, -90 and 90)

	"""list(int): List of coordinates"""
	"""list(float): Coordinates specifying the subregion that will be extracted."""

Download subregion #66

Download subregion #66

Conversation

aytacpacal commented Jan 12, 2021 • edited by Peter9192 Loading

Peter9192 commented Jan 12, 2021

Peter9192 left a comment

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 Jan 15, 2021

Choose a reason for hiding this comment

Peter9192 commented Jan 25, 2021

aytacpacal commented Jan 25, 2021

Peter9192 commented Jan 25, 2021

Peter9192 commented Feb 16, 2021

aytacpacal commented Feb 16, 2021

bvreede commented Feb 18, 2021

aytacpacal commented Feb 18, 2021

bvreede commented Mar 4, 2021

aytacpacal commented Jan 12, 2021 •

edited by Peter9192

Loading