-
Notifications
You must be signed in to change notification settings - Fork 138
Thermosteric sea-level rise patterns #4047
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
base: main
Are you sure you want to change the base?
Conversation
I know that I need a scientific and technical reviewer, I am on the hunt for both :) |
Hey @valeriupredoi! Was wondering if you could give me a hand with some CircleCI tests failing... it's not clear whether it has anything to do with my code or not. Cheers 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your contribution @mo-gregmunday! 🥳
@@ -0,0 +1,96 @@ | |||
# (C) Crown Copyright 2022-2025, Met Office. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in this module looks very similar to what was included via #2785! Would it be possible to add a single copy of this code to esmvaltool/diag_scripts/shared and use the functions for both recipes, please? 😊
description: Generating sterodynamic sea-level patterns from CMIP6 models. | ||
title: Sterodynamic sea-level patterns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation you added uses:
- thermosteric sea-level change patterns from CMIP6 model datasets.
- thermosteric sea-level patterns from CMIP6 models
It would be great if these were all consistent! 😊
fig.savefig( | ||
Path(plot_path) | ||
/ f'detrended_{zostoga.attributes["source_id"]}.png', dpi=150) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could esmvaltool.diag_scripts.shared.save_figure
be used here? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(there are other uses of this in this module!)
idx = (yrs >= start_yr) & (yrs <= end_yr) | ||
zostoga = zostoga[idx] | ||
zos = zos[idx] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability, use index
rather than idx
😊
np.save( | ||
Path(work_path) | ||
/ f"zos_regression_{scenario}_{model}.npy", slopes) | ||
np.save(Path(work_path) / f"zos_mask_{scenario}_{model}.npy", mask) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could esmvaltool.diag_scripts.shared.save_data
be used here? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(there are other uses of this in this module!)
fig: plt.figure | ||
figure of the evaluation | ||
""" | ||
# Plot the mse for each scenario |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next three chunks of code are very similar; would it be possible to consolidate them, please? 😊
|
||
|
||
def prepare_zostoga( | ||
zostoga_list: iris.cube.Cube, plot_path: Path) -> list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring states zostoga_list
is a list
; which is correct?
# Calculate regression between zostoga and zos | ||
scenarios = ["ssp245", "ssp370", "ssp585"] | ||
slopes, masks = [], [] | ||
for i, (z_dtr, zos) in enumerate(zip(zostoga_list, zos_list)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability, use index
rather than i
😊
zos: list | ||
list of zos scenarios | ||
""" | ||
# Satisfying CI linting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to consolidate the code below to something like (untested):
zostoga_names = ["zostoga_picontrol", "zostoga_245", "zostoga_370", "zostoga_585"]
zostoga_cubes = []
for dataset in cfg["input_data"].values():
if dataset["dataset"] == model:
for zostoga_name in zostoga_names:
if dataset["variable_group"] == zostoga_name:
input_file = dataset["filename"]
zostoga_cubes.append(sf.load_cube(input_file))
What do you think? 🤔
@@ -0,0 +1,531 @@ | |||
# (C) Crown Copyright 2022-2025, Met Office. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the years in this copyright statement correct? If you only worked on this code this year, the year should just be 2025
.
@mo-gregmunday, run the following command in the directory containing your branch and commit the changes:
😊 |
To download (almost all!) the required data for this recipe, I needed to use a specific ESGF node, which I did by creating a
I did this both at DKRZ and at the MO 👍 |
Currently missing data:
|
I have opened #4117 for the data issue 👍 |
I tried again today to download data and I was able to download the data from LLNL 🥳 The recipe runs at the Met Office (using ESMValTool v2.12.0):
However, there are no plots visible via the I'm getting a failure on DKRZ:
@mo-gregmunday any thoughts? 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment!
The logs contain multiple copies of the following message:
WARNING:py.warnings:/home/b/b382148/software/ESMValTool-main/esmvaltool/diag_scripts/steric_patterns/steric_patterns.py:280: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`). Consider using `matplotlib.pyplot.close()`.
fig = plt.figure(figsize=(12, 5), layout="constrained")
Would it be possible to implement what is suggested in this message, please? 😊
Description
This PR calculates and evaluates patterns of thermometric sea-level rise from global thermal expansion and local sea-level changes for a range of CMIP6 models and scenarios.
Systematic calculation and evaluation of these patterns will allow for quick and easy pattern creation for future CMIP generations, and allows the patterns to be used in various sea-level rise emulation models.
Before you get started
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 recipe/diagnostic