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.

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

Update variables in inputref.py #84

Merged
merged 6 commits into from
Apr 20, 2021
Merged

Update variables in inputref.py #84

merged 6 commits into from
Apr 20, 2021

Conversation

bvreede
Copy link
Contributor

@bvreede bvreede commented Apr 19, 2021

The current variables in inputref.py do not reflect the current state of the API at CDS. The file has been amended to include more variables, both hourly and monthly, that are available for the ERA5 datasets.

Three variables were in our reference, but not part of the selection at CDS:

  • altimeter_corrected_wave_height
  • altimeter_range_relative_correction
  • altimeter_wave_height

I have left them in (running a quick check with one of these showed that they did in fact result in the download of data), but propose that we take another look at the current set of variables, perhaps consider how we can automate this workflow so we have an up-to-date set of variables in the tool (see #83), and/or consider how much we want era5cli to 'protect' the user from framing an impossible request.

I have also generated a list of variables that is complementary to MISSING_MONTHLY_VARS, as there were several variables in the monthly datasets that were not found in the hourly datasets. However, while the MISSING_MONTHLY_VARS are used in checks inside the tool (to make sure that a user does not create an 'impossible' request with these variables), the MISSING_HOURLY_VARS are not. The existence of this list is, at the moment, purely for documentation.

Closes #77.

@bvreede bvreede requested a review from stefsmeets April 19, 2021 14:52
@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #84 (6547514) into master (4d4fce6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #84   +/-   ##
=======================================
  Coverage   98.31%   98.32%           
=======================================
  Files           7        7           
  Lines         357      358    +1     
=======================================
+ Hits          351      352    +1     
  Misses          6        6           
Impacted Files Coverage Δ
era5cli/inputref.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d4fce6...6547514. Read the comment docs.

Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

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

So, if I understand correctly, there is a master list of variables: SLVARS, and then an additional list of variables which are missing from the master list, so that HOURLY_VARS == SLVARS - MISSING_HOURLY_VARS?

Then, should fetch.py also be updated somewhere here to make use of this new MISSING_HOURLY_VARS dict?

Something like is done here for the monthly vars?

if variable in ref.MISSING_MONTHLY_VARS:

era5cli/inputref.py Outdated Show resolved Hide resolved
@bvreede
Copy link
Contributor Author

bvreede commented Apr 20, 2021

Thanks @stefsmeets, well spotted!

I chose not to do this, as the purpose of MISSING_HOURLY_VARS at the moment is purely for documentation. Implementing this list will have effects on the cli calls done that include these variables; I have not checked whether these variables actually do return data, but they could; implementing this list in the same way as done with MISSING_MONTHLY_VARS will stop the request from being formed altogether. There must be a reason why they're gone from CDS, but e.g. I did find other variables that were no longer on the list on the website, but when used in an era5cli call still return data.

As for the variable list itself: there is a bigger problem there, too, in that this is a (currently purely manual) update that should be done frequently with CDS updates. In other words: this list of variables needs to be refactored into an automated workflow (see also #83), so I did not want to spend too much energy into implementing any downstream refactoring that may become obsolete soon.

The main point for this PR is to make sure there are no bugs in the upcoming release, so I tried to match the variable lists with info on CDS, and added the variable for documentation purposes. I've now tried to make this clearer in the code itself.

@bvreede bvreede requested a review from stefsmeets April 20, 2021 04:31
Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

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

Alright, just making sure it was not an oversight! 🚀

@bvreede bvreede merged commit 807e0bc into master Apr 20, 2021
@bvreede bvreede deleted the update_variables branch April 20, 2021 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update MISSING_MONTHLY_VARS
2 participants