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

CAPE ratio diagnostic #325

Merged
merged 9 commits into from
Dec 15, 2023
Merged

CAPE ratio diagnostic #325

merged 9 commits into from
Dec 15, 2023

Conversation

daflack
Copy link
Contributor

@daflack daflack commented Dec 11, 2023

fixes #324

@daflack daflack self-assigned this Dec 11, 2023
@daflack daflack added enhancement New feature or request science Scientific capabilities labels Dec 11, 2023
Copy link
Contributor

github-actions bot commented Dec 11, 2023

Coverage

@jfrost-mo
Copy link
Member

Rendered Documentation

@jfrost-mo jfrost-mo changed the title cape ratio addition CAPE ratio diagnostic Dec 11, 2023
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Technically looks good. Just one point around whether creating zeroed arrays is necessary as we then overwrite with copied arrays.

src/CSET/operators/convection.py Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Outdated Show resolved Hide resolved
fixes review suggestions

Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
Copy link
Member

@jfrost-mo jfrost-mo 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. I'm not going to mark it as approved yet as it still needs science and portability reviews.

Copy link
Collaborator

@JorgeBornemann JorgeBornemann left a comment

Choose a reason for hiding this comment

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

It looks good, and I can't see this change causing any problem with portability; however, I can't test in our installation at the moment as I don't have any model output with the three diagnostics required (I am working on it).

Two very minor comments:

  1. The help in the metadata for the GUI could specify explicitly the stash codes for the three required model diagnostics, or at least warn the user that model diagnostics other than the 'standard' ones are required.
  2. There is an inconsistency between the default value for MUCIN_thresh between the recipe (-30 J/Kg) and the code (-75 J/Kg), is that intentional?, one would expect that both the recipe and the default will have the most common value.

Approved for portability.

Copy link
Contributor

@jwarner8 jwarner8 left a comment

Choose a reason for hiding this comment

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

Overall looks great, some minor points added to consider

cset-workflow/includes/deterministic_plot_cape_ratio.cylc Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Show resolved Hide resolved
tests/operators/test_convection.py Outdated Show resolved Hide resolved
cset-workflow/includes/deterministic_plot_cape_ratio.cylc Outdated Show resolved Hide resolved
cset-workflow/meta/rose-meta.conf Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Outdated Show resolved Hide resolved
src/CSET/operators/convection.py Show resolved Hide resolved
daflack and others added 2 commits December 15, 2023 08:49
Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
@daflack daflack merged commit 56e0fc6 into main Dec 15, 2023
5 checks passed
@daflack daflack deleted the 324_cape_ratio_addition branch December 15, 2023 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request science Scientific capabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CAPE ratio
4 participants