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

eWaterCycle: Add recipe to prepare input for LISFLOOD #1298

Merged
merged 53 commits into from
May 12, 2020

Conversation

sverhoeven
Copy link
Contributor

@sverhoeven sverhoeven commented Sep 5, 2019

Short description of the diagnostic
Convert ERA-Interim and/or ERA5 meteorological input data to a format that can be used as forcing data for the LISFLOOD hydrological model. This is part of the preprocessing workflow in the eWaterCycle project.


Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • Make sure your code is composed of functions of no more than 50 lines and uses meaningful names for variables
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • [x]~ (Only if really necessary) Add any additional dependencies needed for the diagnostic script to setup.py, esmvaltool/install/R/r_requirements.txt or esmvaltool/install/Julia/julia_requirements.txt (depending on the language of your script) and also to meta.yaml for conda dependencies (includes Python and others, but not R/Julia)~
  • If new dependencies are introduced, check that the license is compatible with Apache2.0

New recipe/diagnostic

  • Add documentation for the recipe to the doc/sphinx/source/recipes folder and add a new entry to index.rst

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Waiting for #1297

Closes #1339

Not working yet, due to missing ERA-Interim vars
@valeriupredoi valeriupredoi changed the title Recipy lisflood Recipe lisflood Sep 9, 2019
@JaroCamphuijsen JaroCamphuijsen changed the title Recipe lisflood eWaterCycle: Add recipe to prepare input for LISFLOOD Oct 2, 2019
@mattiarighi mattiarighi changed the base branch from version2_development to master January 3, 2020 17:59
@Peter9192
Copy link
Contributor

Peter9192 commented Apr 28, 2020

To do:

  • Merge ERA5 and ERA-Interim parts of the recipe (since we now have daily cmorized input data available, we can use exactly the same preprocessors)
  • Add an extract_region preprocessor or something like that (we don't want to regrid the entire globe)
  • Maybe use the lazy_linear_regrid function that Bouwe made for the wflow diagnostic?, took 2 min for preprocssing 1 year per var using 15Gb RAM
  • Implement the diagnostic functionality (right now it's just comments)

@bouweandela
Copy link
Member

e and e0, u and v are clear. According to the PR template of the repository, few remaining quality issues are acceptable. I would like to ask @mattiarighi or @bouweandela opinion on this matter.

I would recommend using PEP8 compliant names, as this is almost no work to fix. While u might be clear to you, a name like eastward_wind is more clear to more people. The remark about a few remaining issues is more about saving scientists hours of work refactoring their code when they have too high complexity/too many variables in a few functions.

@Peter9192
Copy link
Contributor

Peter9192 commented May 11, 2020

I would recommend using PEP8 compliant names, as this is almost no work to fix. While u might be clear to you, a name like eastward_wind is more clear to more people.

Note that u and v are perfectly PEP8 compliant. It's pylint's own opiniated rule that forbids this.
IMHO,

# Pythagorean theorem
a**2 + b**2 = c**2

is much clearer than

hypothenuse**2 = (
    adjacent_side**2 
    + opposite_side**2
)

Image the same example for the quadratic formula:
\Large x=\frac{-b\pm\sqrt{b^2-4ac}}{2a}

Perhaps we can compromise by using uas and vas?

@bouweandela
Copy link
Member

Note that u and v are perfectly PEP8 compliant. It's pylint's own opiniated rule that forbids this.

You're right, it's not PEP8. However, in the ESMValTool project we try to follow the recommendations provided by Codacy for keeping our code readable for everyone. These recommendations are indeed more broad than simply what is recommended in PEP8,

Perhaps we can compromise by using uas and vas?

It's up to you

Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@sverhoeven thanks. Just a few remaining things to be improved.

@SarahAlidoost
Copy link
Member

@Peter9192 I noticed that there are still some Codacy issues (pylint and flake8). Please have a look. Thanks.

sverhoeven and others added 5 commits May 11, 2020 17:13
As area_weighted used to much memory, must use lazy regridding to get a runnable version.
As ValueError: 'actual_vapour_pressure' is not a valid standard_name
@bouweandela bouweandela merged commit 962c481 into master May 12, 2020
@bouweandela bouweandela deleted the recipy_lisflood branch May 12, 2020 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eWaterCycle: Add recipe to prepare input for LISFLOOD
6 participants