Skip to content

Add hydrological forcing comparison recipe#2013

Merged
nielsdrost merged 22 commits into
masterfrom
hydro_forcing
Mar 16, 2021
Merged

Add hydrological forcing comparison recipe#2013
nielsdrost merged 22 commits into
masterfrom
hydro_forcing

Conversation

@stefsmeets

@stefsmeets stefsmeets commented Feb 4, 2021

Copy link
Copy Markdown
Contributor

Description

Hi everyone, this recipe compares between ERA5, ERA-interim, and MSWEP datasets for a certain catchment.

Todo:


Before you get started

Checklist

It is the responsibility of the author to make sure the PR is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated recipe/diagnostic:


To help with the number pull requests:

@stefsmeets stefsmeets changed the title Hydrological forcing agreement recipe [WIP] Hydrological forcing agreement recipe Feb 16, 2021
@stefsmeets stefsmeets changed the title Hydrological forcing agreement recipe Add hydrological forcing comparison recipe Feb 16, 2021
@stefsmeets stefsmeets marked this pull request as ready for review February 16, 2021 09:58
@stefsmeets

Copy link
Copy Markdown
Contributor Author

Hi @Peter9192 , I think this PR is ready. Could you do a technical review?

@stefsmeets stefsmeets requested a review from Peter9192 February 16, 2021 09:59
@jvegreg

jvegreg commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

I can test it if you want, but I will need a suitable shapefile

@stefsmeets

Copy link
Copy Markdown
Contributor Author

I can test it if you want, but I will need a suitable shapefile

@jvegasbsc That would be great! I'm using the shapefiles from here: https://github.com/eWaterCycle/recipes_auxiliary_datasets/tree/master/Lorentz_Basin_Shapefiles

You will also need the MSWEP data: https://docs.esmvaltool.org/projects/ESMValCore/en/latest/develop/fixing_data.html?highlight=mswep#mswep
(or just run with ERA5 / ERA-Interim)

@jvegreg

jvegreg commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

Thanks. I've just run it for ERA-Interim, as we only have monthly data for ERA5 in jasmin

@jvegreg jvegreg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code itself looks good and clean. Good work, @stefsmeets

Comment thread esmvaltool/diag_scripts/hydrology/hydro_forcing.py Outdated
@stefsmeets stefsmeets requested a review from jvegreg February 18, 2021 14:18
@stefsmeets

Copy link
Copy Markdown
Contributor Author

Thanks @jvegasbsc , I have updated the code. I think it is better now 👍

@stefsmeets

Copy link
Copy Markdown
Contributor Author

Hi @jeromaerts , could you do a scientific review of this recipe? Especially, could you have a look at the documentation, descriptions, captions, etc., whether they make sense scientifically?

@jeromaerts

Copy link
Copy Markdown
Contributor

@stefsmeets, Looks great, also from a scientific point of view.

Some small notes on the documentation:

  • "3. Plot the monthly climate statistics over a certain period"
    Change: "3. Plot the monthly climatology statistics over a certain period"

If possible extent the plotting of climatology to also include a daily climatology plot.

  • "All hydrological recipes require a shapefile as an input to produce forcing data."
    Change, produce to select.

  • "All recipes are tested with the shapefiles that are used for the eWaterCycle project."
    "All recipes are tested with shapefiles from HydroSHEDS that are used for the eWaterCycle project."

Looking at the figures, the dates on the horizontal axis are very close to each other, perhaps slightly reduce the label font-size.

@stefsmeets

Copy link
Copy Markdown
Contributor Author

Hi @jeromaerts , thanks for the comments! I have updated the code and added the new plot. Let me know what you think. If you are happy with the changes, could you approve the PR?

@jeromaerts jeromaerts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The docs are updated and consistent with hydrology conventions.

@stefsmeets

stefsmeets commented Feb 24, 2021

Copy link
Copy Markdown
Contributor Author

Hi @jvegasbsc , have you had a chance to take another look at the changes I made? If you agree, could you approve this PR so we can get this merged? If you have any other comments, please let me know!

@stefsmeets

Copy link
Copy Markdown
Contributor Author

ping @jvegasbsc 😁

@jvegreg jvegreg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks better now! I agree with @nielsdrost comment about tha nimng of the entry_point but that's a very minor concern.

And sorry for the delay, I went straight to holidays after the release

@stefsmeets

Copy link
Copy Markdown
Contributor Author

Looks better now! I agree with @nielsdrost comment about tha nimng of the entry_point but that's a very minor concern.

And sorry for the delay, I went straight to holidays after the release

No worries, probably well-deserved! 😅 Will fix it next week 👍

@stefsmeets stefsmeets requested a review from nielsdrost March 15, 2021 09:37
@nielsdrost nielsdrost merged commit 8775ced into master Mar 16, 2021
@nielsdrost nielsdrost deleted the hydro_forcing branch March 16, 2021 09:31
@nielsdrost

Copy link
Copy Markdown
Member

Thanks @stefsmeets !

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.

7 participants