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

Update monitor diagnostics #2608

Merged
merged 15 commits into from
May 9, 2022
Merged

Update monitor diagnostics #2608

merged 15 commits into from
May 9, 2022

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Mar 18, 2022

Description

This PR updates the monitoring diagnostic.

  • Update the default plot path in the documentation so that is reflects the true default path (I didn't change the actual path) and allow the usage of the tag {plot_dir} (default ESMValTool plot directory) in the recipe.
  • Always use the user-defined output-file-type for all plots.
  • Added automatic rasterization for map plots that can be turned off using the option rasterize_maps=False (default True). Note that I had to implement this via Artist.set_rasterized(True) since it's not possible to pass arbitrary keyword arguments (and thus rasterized=True) to the plotting function in mapgenerator.
  • Added captions to the provenance record of all plots.
  • For timeseries plots, I think the original intention was to also include a smoothed version of the timeseries in the same plot for longer time series. However, the way it was coded, only the smoothed version was plotted, since the plot is saved and closed after each call of plot_cube. I changed that so that the smoothed version gets plotted in a separate plot. Feel free to suggest any alternatives, but note that maybe an update of mapgenerator is necessary for that.
  • Adapted some default options for suptitle positions because the titles were sometimes overlapping, especially for stereographic plots.

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:

@schlunma schlunma added this to the v2.6.0 milestone Mar 18, 2022
@schlunma schlunma self-assigned this Mar 18, 2022
@schlunma
Copy link
Contributor Author

schlunma commented May 2, 2022

Could anyone take a look at this? It would be nice to merge this soon since I have a follow-up PR that extends the monitoring diagnostic with an additional plot type.

@sloosvel
Copy link
Contributor

sloosvel commented May 2, 2022

apologies, but I am a bit packed with another couple of diags! If you are in a hurry feel free to ask someone else to review, else I'll take a look as soon as I am done with those tasks.

@schlunma
Copy link
Contributor Author

schlunma commented May 2, 2022

No worries, just wanted to make sure that we don't forget about this!

@sloosvel
Copy link
Contributor

sloosvel commented May 6, 2022

Taking a look on this on monday! sorry for the delay

@sloosvel
Copy link
Contributor

sloosvel commented May 9, 2022

For timeseries plots, I think the original intention was to also include a smoothed version of the timeseries in the same plot for longer time series. However, the way it was coded, only the smoothed version was plotted, since the plot is saved and closed after each call of plot_cube. I changed that so that the smoothed version gets plotted in a separate plot. Feel free to suggest any alternatives, but note that maybe an update of mapgenerator is necessary for that.

Having the smoothed versions in a separate plot is fine for me. I also spoke to the maintainer of mapgenerator, if any of you wants to contribute changes for the monitor diags into the package feel free to do so. Just don't expect a lot of involvement to keep up with ESMValTool needs because it's an internal package that is mostly used by other groups in the dept, but if you feel like contributing go for it.

Copy link
Contributor

@sloosvel sloosvel left a comment

Choose a reason for hiding this comment

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

Overall the changes look fine to me and do not break anything. I could run the recipe setting several output files types without any kind of issue. As mentioned, if you want to contribute the plot changes to mapgenerator you can, but it's not necessary.

@schlunma
Copy link
Contributor Author

schlunma commented May 9, 2022

Thanks for the review Saskia! 🎉

As mentioned, if you want to contribute the plot changes to mapgenerator you can, but it's not necessary.

I agree, that shouldn't be necessary.

@schlunma schlunma merged commit aa6335a into main May 9, 2022
@schlunma schlunma deleted the update_monitor branch May 9, 2022 15:01
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.

Format of output plots in recipe_monitor.yml is sometimes hardcoded instead of configurable
2 participants