Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix start years #123

Merged
merged 9 commits into from
Nov 29, 2022
Merged

Fix start years #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bc485c1
changing start year to 1959 for ERA5 and 1950 for ERA5-Land, removing…
Oct 4, 2022
23edd96
changing version number, adding Liza to the list of authors
Oct 4, 2022
440d233
revert CITATION.cff to the previous release date
malininae Oct 24, 2022
73b30d5
revert CITATION.cff to the previous number
malininae Oct 24, 2022
56bd56b
Removing Liza from era5cli/__version__.py
malininae Oct 24, 2022
b1b98dd
Update era5cli/cli.py
malininae Oct 24, 2022
8355950
updating tests for --land and years out of range
Oct 25, 2022
ac29aed
Update era5cli/__version__.py
malininae Oct 25, 2022
fde53b0
modifying message if ERA5-Land is used with the back extension
Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .zenodo.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@
"affiliation": "Netherlands eScience Center",
"name": "Verhoeven, Stefan",
"orcid": "0000-0002-5821-2060"
},
{
"affiliation": "Environment and Climate Change Canada",
"name": "Malinina, Elizaveta",
"orcid": "0000-0002-4102-2877"
}
],
"description": "A command line interface to download ERA5 data from the Climate Data Store.\n",
Expand Down
5 changes: 5 additions & 0 deletions CITATION.cff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ authors:
family-names: Verhoeven
given-names: Stefan
orcid: https://orcid.org/0000-0002-5821-2060
-
affiliation: "Environment and Climate Change Canada"
family-names: Malinina
given-names: Elizaveta
orcid: https://orcid.org/0000-0002-4102-2877

cff-version: 1.2.0
date-released: 2021-11-30
Expand Down
13 changes: 7 additions & 6 deletions era5cli/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ def _build_parser():
Whether to download the preliminary back extension
(1950-1978). Note that when `--prelimbe` is used,
`--startyear` and `--endyear` should be set
between 1950 and 1978.
between 1950 and 1978. Please, be aware that
ERA5 data is available from 1959.
`--prelimbe` is incompatible with `--land`.

''')
Expand All @@ -159,7 +160,7 @@ def _build_parser():
help=textwrap.dedent('''\
Whether to download data from the ERA5-Land
dataset. Note that the ERA5-Land dataset starts in
1981.
1950.
`--land` is incompatible with the use of
`--prelimbe` and `--ensemble`.

Expand Down Expand Up @@ -343,12 +344,12 @@ def _construct_year_list(args):
'year should be between 1950 and 1978'
)
elif args.land:
assert 1981 <= year <= datetime.now().year, (
'for ERA5-Land, year should be between 1981 and present'
assert 1950 <= year <= datetime.now().year, (
'for ERA5-Land, year should be between 1950 and present'
)
else:
assert 1979 <= year <= datetime.now().year, (
'year should be between 1979 and present'
assert 1959 <= year <= datetime.now().year, (
'year should be between 1959 and present'
)

assert endyear >= args.startyear, (
Expand Down
3 changes: 2 additions & 1 deletion era5cli/fetch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,8 @@ def _build_name(self, variable):
if self.prelimbe:
if self.land:
raise ValueError(
"Back extension not (yet) available for ERA5-Land.")
"Back extension not available for ERA5-Land. "
"ERA5-Land data is available from 1950 on.")
name += "-preliminary-back-extension"
return name, variable

Expand Down
10 changes: 1 addition & 9 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def test_main_fetch(fetch):
assert cli._execute(args)

# should give an AssertionError if years are out of bounds
argv = ['hourly', '--startyear', '1950',
argv = ['hourly', '--startyear', '1949',
'--variables', 'total_precipitation', '--statistics',
'--endyear', '2007', '--ensemble']
args = cli._parse_args(argv)
Expand All @@ -160,14 +160,6 @@ def test_main_fetch(fetch):
args = cli._parse_args(argv)
cli._execute(args)

# 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)

Comment on lines -163 to -170
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw @bvreede is on holiday until next week

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for figuring this out; indeed, that's a good idea, let's do it!


@mock.patch("era5cli.info.Info", autospec=True)
def test_main_info(info):
Expand Down