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

recreate perfmetrics portrait plot in python #3551

Open
wants to merge 63 commits into
base: main
Choose a base branch
from
Open

Conversation

lukruh
Copy link
Contributor

@lukruh lukruh commented Mar 20, 2024

Description


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:

@valeriupredoi
Copy link
Contributor

what a noble venture, Lukas! Godspeed 🏁 🍺

@lukruh lukruh added this to the v2.11.0 milestone Apr 16, 2024
@lukruh lukruh added the new recipe Use this label if you are adding a new recipe label Nov 21, 2024
@lukruh lukruh marked this pull request as ready for review November 21, 2024 10:52
Copy link
Contributor

@bettina-gier bettina-gier left a comment

Choose a reason for hiding this comment

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

Some minor changes needed, mostly in documentation.

Also requires ESMValGroup/ESMValCore#2564 to run with a current core bug on multi-model statistics.

doc/sphinx/source/recipes/recipe_perfmetrics.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_portrait.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_portrait.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_portrait.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_portrait.rst Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/portrait_plot.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/portrait_plot.py Outdated Show resolved Hide resolved
esmvaltool/recipes/recipe_portrait_CMIP.yml Outdated Show resolved Hide resolved
esmvaltool/recipes/recipe_portrait_CMIP.yml Outdated Show resolved Hide resolved
esmvaltool/recipes/recipe_portrait_CMIP.yml Outdated Show resolved Hide resolved
@lukruh lukruh added the requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. label Nov 21, 2024
@lukruh
Copy link
Contributor Author

lukruh commented Nov 22, 2024

The recipe runs again with ESMValGroup/ESMValCore#2564. Should we wait for it before merging this?

@schlunma I adressed all your review comments. If you have time to re-review it, that would be awesome.

I added a cheaper version of the recipe to the recipes/testing folder. Is that the correct way to provide a test version? Should I also add it to RTW?

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thanks @lukruh and @diegokam, this is really awesome! I got a couple of small comments on the documentation. After that I think it's good to go. 🚀

Unfortunately, I fear that we can only merge this once we have a new ESMValCore release (candidate); otherwise the doc build will fail. I am honestly a bit surprised that the tests pass; usually they fail if you use a new preprocessor that is not available in the stable version..

and grouping all datasets by project.

To plot distance metrics like RMSE, pearson R, bias etc. the
:func:`distance_metrics <esmvalcore.preprocessor.derive>` preprocessor or
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:func:`distance_metrics <esmvalcore.preprocessor.derive>` preprocessor or
:func:`distance_metric <esmvalcore.preprocessor.distance_metric>` preprocessor or

.. toctree::
:maxdepth: 1

recipe_perfmetrics
recipe_portrait
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should appear under General-purpose diagnostics.

Comment on lines +7 to +9
Plot performance metrics of multiple datasets vs up to four references
A :doc:`documented example recipe </recipes/recipe_portrait>` to use this
diagnostic is provided as `recipes/recipe_portrait_CMIP.yml`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go into the docstring of the Python module.

diagnostic is provided as `recipes/recipe_portrait_CMIP.yml`.

.. automodule:: esmvaltool.diag_scripts.portrait_plot
:members:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:members:
:no-members:
:no-inherited-members:
:no-show-inheritance:

Do you think it makes sense to use the functions of this module outside of it? If not, I wouldn't show them.

split_by: str, optional
The rectangles can be split into 2-4 triangles. This is used to show
metrics for different references. For this case there is no need to change
this parameter. Multiple variables can be set in the recipe with `split`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this parameter. Multiple variables can be set in the recipe with `split`
this parameter. Multiple variables can be set in the recipe with ``split``

Single backticks are rendered italic, to get monospace font you need double backticks. Applies not only to this line.

E.g. label, ticks...
By default {}.
plot_properties: dict, optional
Dictionary that gets passed to `matplotlib.axes.Axes.set()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Dictionary that gets passed to `matplotlib.axes.Axes.set()`.
Dictionary that gets passed to :meth:`matplotlib.axes.Axes.set`.

This will create a link. Applies not only to this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostic new recipe Use this label if you are adding a new recipe requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recreate portrait plot from NCL perfmetrics in python
8 participants