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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMORizer for CLARA-AVHRR cloud data #2101

Merged
merged 6 commits into from Jun 28, 2021
Merged

CMORizer for CLARA-AVHRR cloud data #2101

merged 6 commits into from Jun 28, 2021

Conversation

axel-lauer
Copy link
Contributor

@axel-lauer axel-lauer commented Mar 24, 2021

This PR add a CMORizer script for the CLARA-AVHRR cloud dataset (variables clt, clivi, clwvi) from the Satellite Application Facility on Climate Monitoring (CM SAF).

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 馃洜 Technical or 馃И Scientific review.

New or updated data reformatting script

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Hi @axel-lauer although I'm not well versed in NCL, I had a go at doing a tech review for this PR. So I just ask a couple of questions to verify I understand the code correctly.

I can tick all tech review boxes since the docs are okay and the variable has been added. It would be good if the scientific reviewer is extra keen on verifying that the output data also makes sense.

@axel-lauer
Copy link
Contributor Author

All changes discussed above have been implemented with commit a755bca.

@valeriupredoi
Copy link
Contributor

cheers @axel-lauer - since I don't speak NCL, I'll let Peter and Remi green flag this PR, one prickly point though - please tick or remove all boxes in the PR description (the units one is left unchecked) 馃嵑

@axel-lauer
Copy link
Contributor Author

@valeriupredoi thanks for taking a look! Just checked the last box.

@valeriupredoi
Copy link
Contributor

yay! if @remi-kazeroni would find a bit of time to run this then I can merge today 馃嵑

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

The cmorizer script runs fine for me but the recipe_check_obs,yml does not. It seems that different variable names (lwp, clwvi) are used. Could you please clarify that @axel-lauer?

doc/sphinx/source/input.rst Outdated Show resolved Hide resolved
esmvaltool/recipes/examples/recipe_check_obs.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks @axel-lauer! This looks ready to be merged. May I let you put the cmorized data into the OBS directory?

@axel-lauer
Copy link
Contributor Author

@remi-kazeroni Thanks! Yes, I'll put the cmorized data into our OBS directory.

@remi-kazeroni
Copy link
Contributor

@axel-lauer is this good to be merged or do you think another more scientific review is needed here?

@axel-lauer
Copy link
Contributor Author

I think this is ready to be merged.

@remi-kazeroni remi-kazeroni merged commit 51f92b8 into main Jun 28, 2021
@remi-kazeroni remi-kazeroni deleted the cmorize_clara-avhrr branch June 28, 2021 09:46
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.

None yet

4 participants