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

Recipe to plot generic output from the preprocessor #2184

Merged
merged 47 commits into from
Feb 25, 2022
Merged

Recipe to plot generic output from the preprocessor #2184

merged 47 commits into from
Feb 25, 2022

Conversation

jvegreg
Copy link
Contributor

@jvegreg jvegreg commented May 29, 2021

This recipe introduce a diagnostic that can plot in a configurable way some direct outputs from the preprocessor. A version of this recipe is what we are using at BSC to monitor our model runs but can be used also for exploratory analysis of already run experiments.

Still missing from this recipe, to be fixed before merge

  • Provenance recording
  • Documentation
  • Release the next version of MapGenerator to solve some graphic issues

Some comments:

  • The diagnostic is meant to grow in the future with new types of plots
  • Plots are stored in a central location because this recipe is mean to be run multiple times (because the experiment is still running or to cover different variables / plot some other things). Storing them in the plot folder of the recipe will have users looking through multiple folders, adding a lot of confusion for no real gain.
  • Plots are overwritten without warning: this is intended for the monitoring use, as we would like to replace the older plots with the newer ones.
  • Actual path and filename of the plots is configurable on the recipe so users can choose where and how store their plots. This is also mean to facilitate collaboration between colleagues and interacting with presentation apps

Documentation: https://esmvaltool--2184.org.readthedocs.build/en/2184/recipes/recipe_monitor.html

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

Javier Vegas-Regidor added 2 commits May 28, 2021 11:26
@jvegreg
Copy link
Contributor Author

jvegreg commented May 31, 2021

One question, @stefsmeets : the html output is expected to fail if some of the recipe products are outside the recipe folder or that is an unexpected behaviour?

@stefsmeets
Copy link
Contributor

stefsmeets commented May 31, 2021

@jvegasbsc What do you mean by 'fail'? The html output uses the provenance system to get the paths to where the plots / data are located. I understood these would always be within the recipe output directory, so the paths are relative to the root directory. I don't know what happens if they are outside 😅

@jvegreg
Copy link
Contributor Author

jvegreg commented May 31, 2021

@jvegasbsc What do you mean by 'fail'? The html output uses the provenance system to get the paths to where the plots / data are located. I understood these would always be within the recipe output directory, so the paths are relative to the root directory. I don't know what happens if they are outside 😅

Now you know:

Traceback (most recent call last):
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/_main.py", line 433, in run
    fire.Fire(ESMValTool())
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/site-packages/fire/core.py", line 141, in Fire
    component_trace = _Fire(component, args, parsed_flag_args, context, name)
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/site-packages/fire/core.py", line 466, in _Fire
    component, remaining_args = _CallAndUpdateTrace(
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/site-packages/fire/core.py", line 681, in _CallAndUpdateTrace
    component = fn(*varargs, **kwargs)
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/_main.py", line 410, in run
    process_recipe(recipe_file=recipe, config_user=cfg)
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/_main.py", line 105, in process_recipe
    recipe.write_html_summary()
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/_recipe.py", line 1446, in write_html_summary
    output.write_html()
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/experimental/recipe_output.py", line 145, in write_html
    html_dump = self.render(template=template)
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/experimental/recipe_output.py", line 161, in render
    rendered = template.render(
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/site-packages/jinja2/environment.py", line 1090, in render
    self.environment.handle_exception()
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/site-packages/jinja2/environment.py", line 832, in handle_exception
    reraise(*rewrite_traceback_stack(source=source))
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/site-packages/jinja2/_compat.py", line 28, in reraise
    raise value.with_traceback(tb)
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/experimental/templates/recipe_output_page.j2", line 12, in top-level template code
    {% include 'RecipeOutput.j2' %}
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/experimental/templates/RecipeOutput.j2", line 3, in top-level template code
    {% include 'TaskOutput.j2' %}
  File "/home/Earth/jvegas/git/ESMValCore/esmvalcore/experimental/templates/TaskOutput.j2", line 9, in top-level template code
    <img width='80%' src='{{ file.path.relative_to(session.session_dir) }}' alt='{{ file.caption }}'/>
  File "/home/Earth/jvegas/.conda/envs/esmvaltool/lib/python3.9/pathlib.py", line 928, in relative_to
    raise ValueError("{!r} is not in the subpath of {!r}"
ValueError: '/home/Earth/jvegas/plots/EC-Earth3-CC/piControl/seaIce/sivol/monclimarctic_sivol_EC-Earth3-CC_SImon_piControl_r1i1p1f1.png' is not in the subpath of '/esarchive/scratch/jvegas/recipe_monitor_20210531_102719' OR one path is relative and the other is absolute.

I will open an issue later to discuss how to manage this, but now I need to go cooking!

@stefsmeets
Copy link
Contributor

stefsmeets commented May 31, 2021

@jvegasbsc Yeah, best to make an issue for that. This fails because if the output plot is not in a subdirectory of the recipe output. This is to guarantee that the html still points to the right data / png files when the directory is moved.

I wonder if it is possible to keep the plots/data inside the directory, and simply link to it from the outside directory.

@jvegreg
Copy link
Contributor Author

jvegreg commented May 31, 2021

The job I use directly deletes the recipe folder if everything works so that is not possible.

@stefsmeets
Copy link
Contributor

Can you add a link the other direction then? 😅

@sloosvel
Copy link
Contributor

@valeriupredoi I have reworked a bit the documentation and added comments in the eofs script, is it clearer now?

@sloosvel
Copy link
Contributor

If this diag cannot make it, can we at least get mapgenerator into the tool?

@valeriupredoi
Copy link
Contributor

If this diag cannot make it, can we at least get mapgenerator into the tool?

looking at the changes now, Saskia, thanks much for them! We will get this in - no worries, we have a week before the big release, and even then I suspect there may be wiggles with other bits of the Tool that may delay the release by few days. This is of very good contributing nature so the rules can be bent a bit for it to get in, right @schlunma ? 😁

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.

couple minor bits but overall rather happy, many thanks @sloosvel 🍺 @schlunma would you maybe have time to test this plss? Also, we need to remember to include mapgenerator>=1.0.5 in the conda recipe 👍

doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
@schlunma
Copy link
Contributor

Having a look at this now

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.

Really nice addition, thanks @sloosvel! I run the recipe successfully (also added other models from other projects). Some of the plots have overlapping text, but since this is highly customizable I don't think that's an issue now.

Apart from very minor comments on the doc, I have two suggestions/questions:

  1. I think it might be great to mention in the doc that this recipe can plot arbitrary model output. This is a feature that many users asked for. Maybe in the FAQ? Maybe something like "Can ESMValTool plot arbitrary model output?" and a link to this recipe?
  2. I noticed that some plots are in .svg format even though I use output-file-type=png. It looks like this is hardcoded in some parts of the diagnostics. What's the reason for that? I think we should stick to the format the user asks for.

doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
----------

* plots:
a dictionary containing the plots to make, with its own options.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you referring to the plot types listed above and the options listed below? Maybe add a link to the section above (List of plot types available in monitor.py) and below (`Plot specific options´) if that's the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really nice addition, thanks @sloosvel! I run the recipe successfully (also added other models from other projects). Some of the plots have overlapping text, but since this is highly customizable I don't think that's an issue now.

Javi's work mostly!

I think it might be great to mention in the doc that this recipe can plot arbitrary model output. This is a feature that many users asked for. Maybe in the [FAQ]
(https://github.com/ESMValGroup/ESMValTool/blob/main/doc/sphinx/source/faq.rst)? Maybe something like "Can ESMValTool plot arbitrary model output?" and a link to this recipe?

Good idea, I will add that

I noticed that some plots are in .svg format even though I use output-file-type=png. It looks like this is hardcoded in some parts of the diagnostics. What's the reason for that? I think we should stick to the format the user asks for.

Can't think of any reason, I guess is something pending to be cleaned. Will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that some plots are in .svg format even though I use output-file-type=png. It looks like this is hardcoded in some parts of the diagnostics. What's the reason for that? I think we should stick to the format the user asks for.

Can't think of any reason, I guess is something pending to be cleaned. Will update.

They are hardoced for size and looks reasons:

  • svg can be zoomed in and out without losing quality, so it is prefered by default
  • svg maps are enormous, so it had to be done in png format. Timeseries on the other hand are smaller in svg than png, so they were a better fit for the online app this was originally for.

Copy link

Choose a reason for hiding this comment

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

On the second point, probably rasterization would be a perfect fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the second point, probably rasterization would be a perfect fit.

I think that is what is done when saving the files as png

Copy link
Contributor

Choose a reason for hiding this comment

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

too little time

I do have very little time right now, sorry.

Copy link
Contributor

@valeriupredoi valeriupredoi Feb 25, 2022

Choose a reason for hiding this comment

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

@zklaus welcoming back Javi, and offering support for your rasterization idea - but I reckon we should not dabble around this just here, I think this whole show will need quite a bit more work to be done in various follow-up PRs, so maybe we can discuss all this there - this is big and functional enough (alas, an excellent contribution!) to be merged now

Copy link

Choose a reason for hiding this comment

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

Understandable, @sloosvel. Could you open an issue about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in #2561

Copy link

Choose a reason for hiding this comment

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

I took the liberty of assigning the 2.6.0 milestone 😊

doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
doc/sphinx/source/recipes/recipe_monitor.rst Outdated Show resolved Hide resolved
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.

One small additional comment, after that I would merge that! Thanks @sloosvel and @jvegreg 👍

@@ -108,3 +108,11 @@ a symbolic link to it so it gets picked up at every re-run iteration:
.. way to have this mode run. The information provided will help you obtain any data
.. that is missing and/or create fixes for the datasets and variables that failed the
.. CMOR checks and could not be fixed on the fly.


Can the ESMValTool plot arbitrary model output
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
Can the ESMValTool plot arbitrary model output
Can the ESMValTool plot arbitrary model output?



Can the ESMValTool plot arbitrary model output
==============================================
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
==============================================
===============================================

@zklaus
Copy link

zklaus commented Feb 25, 2022

Great addition!

One small comment, not worth any hold-up: I think we should consistently use "ESMValTool" as a proper name, that means no articles in front of it. In other words, it should never be "the ESMValTool", but only "ESMValTool".

@valeriupredoi
Copy link
Contributor

Great addition!

One small comment, not worth any hold-up: I think we should consistently use "ESMValTool" as a proper name, that means no articles in front of it. In other words, it should never be "the ESMValTool", but only "ESMValTool".

very good point! I have noticed (and been mildly bothered) by this very many times with a lot of the writeups about the ESMValTool 😁

@schlunma schlunma merged commit b68efbf into main Feb 25, 2022
@schlunma schlunma deleted the monitoring branch February 25, 2022 14:08
@sloosvel
Copy link
Contributor

Thanks!!!

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.

6 participants