Skip to content

Conversation

@sloosvel
Copy link
Contributor

@sloosvel sloosvel commented Sep 4, 2019

Description

Recipe and diagnostic script to compute the Eady Growth Rate (PRIMAVERA)


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:


@sloosvel sloosvel added PRIMAVERA Metric from the PRIMAVERA project diagnostic labels Sep 4, 2019
@sloosvel sloosvel marked this pull request as ready for review October 16, 2019 10:48
@sloosvel sloosvel requested a review from jvegreg October 16, 2019 10:56
Co-Authored-By: Bouwe Andela <bouweandela@users.noreply.github.com>
@sloosvel sloosvel mentioned this pull request Oct 18, 2019
1 task
@sloosvel
Copy link
Contributor Author

Some of the models in the recipe require fixes, do I need to open an issue in the core? Or can I directly open a pull request? I already had them collected in a branch.

@bouweandela
Copy link
Member

Some of the models in the recipe require fixes, do I need to open an issue in the core? Or can I directly open a pull request? I already had them collected in a branch.

It would be best if you could do both issue and pull request, because that way it is more visible to others that this is 1) a known issue and 2) you are working on it so they don't need to do it.

@stefsmeets stefsmeets self-requested a review November 26, 2020 11:02
@stefsmeets
Copy link
Contributor

stefsmeets commented Nov 26, 2020

I think most of the comments have been adressed, not sure how to fix the remaining codacy issues.

  • Missing module docstring (missing-module-docstring): The ESMValTool style guide asks us to add a short one line string to the module. Basically add something like this to the top of the file:
"""Diagnostic for PRIMAVERA Eady Growth Rate"""

(make sure to add a blank line after to avoid another style issue)

  • Missing class docstring (missing-class-docstring): basically classes also need a docstring, is it missing for EadyGrowthRate?

  • Class 'EadyGrowthRate' inherits from object, can be safely removed from bases in python3 : Means that inheriting from object is redundant: class EadyGrowthRate(object): -> class EadyGrowthRate:

  • Too many instance attributes (10/7): Feel free to ignore that one 😅

  • No blank lines allowed after function docstring (found 1) (D202): Seems to me like the docstring for __init__ has a blank line too many.

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.

Looks good to me! Just some minor formatting suggestions from my side!

Link to documentation: https://esmvaltool--1285.org.readthedocs.build/en/1285/recipes/recipe_eady_growth_rate.html

Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
@bouweandela
Copy link
Member

Scientific review-wise, I had people in our department making sure that the results make sense and are actually using them for their own work. Is that enough or would it be better to ask them to comment here?

Once you're done with all the technical work, I think it might be nice ask them to comment (and approve) here, to make it a bit more visible. It would be great if they could try out the new scientific review checklist.

@emchamarro
Copy link

emchamarro commented Feb 15, 2021

Hello,

The description looks fine. I think we could complete it with a reference:

documentation:
  description: |
    Recipe to compute the annual mean or the seasonal mean of the maximum Eady Growth Rate (EGR;
Brian J Hoskins and Paul J Valdes. On the existence of storm-tracks. Journal of the atmospheric sciences, 47(15):1854–1864, 1990.)
   The output produces netcdf files for each model. In the seasonal means, a plot is
    produced for each specified level showing the EGR values over the North-Atlantic region.

The plots look fine, but we could adapt the color scale for values between 0 and 1.1 or so, to see better the signal over the Atlantic. But this is just a suggestion.

I would approve the merge of this recipe.

@sloosvel
Copy link
Contributor Author

Thank you for the comments @emchamarro! Added the suggestions.

@sloosvel
Copy link
Contributor Author

@jvegasbsc can you take a look at this, I am not sure why some tests suddenly do not pass. Other than that I would say that everything is ready now?

@jvegreg
Copy link
Contributor

jvegreg commented Feb 15, 2021

Merge master, I think you need #2022

@sloosvel
Copy link
Contributor Author

It's already up do date. Do I need to do anything else to get this approved?

@jvegreg
Copy link
Contributor

jvegreg commented Feb 15, 2021

I got the same issues in other branches, so don't care

@sloosvel sloosvel changed the title PRIMAVERA Eady Growth Rate Add PRIMAVERA Eady Growth Rate diagnostic Feb 16, 2021
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.

PRIMAVERA Eddy Growth Rate

6 participants