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

Implied heat transport new diagnostic #3177

Merged
merged 67 commits into from Nov 2, 2023

Conversation

mo-abodas
Copy link
Contributor

@mo-abodas mo-abodas commented May 16, 2023

Description

This recipe implements the analysis documented in Pearce and Bodas-Salcedo (2023). If CERES-EBAF is used as input dataset then it produces the results in that paper, albeit with small differences. The differences are due to the Edition of the dataset. The paper uses the latest edition (Ed4.1), whereas the recipe uses an older edition available in obs4MIPs (Ed2.7).


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


To help with the number of pull requests:

@mo-abodas
Copy link
Contributor Author

mo-abodas commented Oct 27, 2023

@ehogan @bouweandela thanks for your reviews! I believe I've addressed all your comments, please let me know if you think anything else is needed.

@ehogan , please see below specific responses to your general comments:

Would you consider using more explicit variable names? There are many three letter variable names (e.g. efp, nnn, vvv, zzz, etc.) that do not mean anything to a technical person like me! Also, I would recommend using the more readable point rather than pnt (which I assume means point!) in favour of saving 2 characters (readability counts!)

I've done this in commits 3a56318, ee0a670, 6306ff2. I've expanded most of the three-letter names with more meaningful names. However, there are a couple of exceptions where I think it's better to keep the original names. I've kept efp and mht as variable names and defined the acronyms in the docstring of the ImpliedHeatTransport class. In poisson_solver.py, I've kept the original names in the methods SphericalPoisson.set_matrices and SphericalPoisson.solve. The reason for this is that the short names are more traceable to the names in the paper that describes the numerical algorithm.

There are a number of calls to print() within the single_model_diagnostics.py diagnostic file; given you have initialised a logger, would it be worth using the logger instead?

Done in 449e2e9.

Would it be possible for you to expand your docstrings to include a description of the arguments, please? (There are examples on how to do this in the radiation budget recipe).

Done in 4b44c60, 417f0a4.

"The preferred way of wrapping long lines is by using Python’s implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation.", see PEP8. You can use something like Black to auto-format the diagnostic files to see how to do this (if you use black on the whole file, the ESMValTool pre-commit hooks will make changes / the checks will require some changes).

Done in 2f54ae3.

I noticed there are a number of occurances of cube.data; running this command will realise the data. Is it possible to use cube instead to enable the benefits of Dask?

Done in f4f8c58. Exceptions are calls to matplotlib plotting functions that require numpy arrays as arguments.

There are a number of "magic numbers" used in the diagnostic files; would it be possible to clarify what they are, please? Adding a comment or a descriptive variable are a couple of examples of how to achieve this 😊 (I have commented on two examples)

Done in 000206c, 015eec3, 40b6081.

There are a number of copys used in the diagnostic files; are they all necessary?

Only the one in the poisson solver. I've replaced the others.
Done in 9c64e8e.

Is it possible to use an already existing Poisson solver, for example, the one in SciPy: https://docs.scipy.org/doc/scipy/reference/generated/scipy.stats.poisson.html?

That is something completely different, it provides samples of a Poisson distribution. The solver in this recipe solves the Poisson differential equation over the sphere.

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

refreshing my stale review - my worries were just numba-related, all good, serious heavy-lifting review done by others 🍺

@ehogan
Copy link
Contributor

ehogan commented Nov 1, 2023

@ehogan @bouweandela thanks for your reviews! I believe I've addressed all your comments, please let me know if you think anything else is needed.

Nice work @alejandrobodas! πŸ₯³

There are a number of calls to print() within the single_model_diagnostics.py diagnostic file; given you have initialised a logger, would it be worth using the logger instead?

Done in 449e2e9.

Should the print on L986 also be updated?

CITATION.cff Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_iht_toa.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_iht_toa.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Many thanks for all your hard work on addressing our review comments, @alejandrobodas! πŸ₯³ I spotted a few minor things 😊

@mo-abodas
Copy link
Contributor Author

@ehogan thanks for spotting a few mistakes! I've fixed them as suggested. I've deleted the print statement because it didn't really provide any useful information.

Copy link
Contributor

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Thanks @alejandrobodas! πŸ₯³

@zklaus zklaus merged commit 2fe223c into main Nov 2, 2023
6 checks passed
@zklaus zklaus deleted the implied_heat_transport_new_diagnostic branch November 2, 2023 09:42
@alistairsellar
Copy link
Contributor

πŸ₯³ Well done @mo-abodas and everyone who helped with this!

@mo-abodas
Copy link
Contributor Author

Thanks everyone for your help!

jvegreg pushed a commit that referenced this pull request Jan 14, 2024
Co-authored-by: Francesca Pearce <francesca.pearce@metoffice.gov.uk>
Co-authored-by: alejandrobodas <alejandroxtr@gmail.com>
Co-authored-by: Emma Hogan <ehogan@users.noreply.github.com>
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.

New diagnostics of implied heat transports from TOA fluxes
8 participants