Deprecate defining write_plots and write_netcdf in config-user file #808

bouweandela · 2020-10-02T13:32:53Z

Deprecate use of write_plots, write_netcdf, output_file_type config-user file

Please define those settings in the diagnostic script setting of the recipe instead, for those diagnostic scripts that support these settings

See #93

Tasks

Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
This pull request has a descriptive title that can be used in a changelog
~~Add unit tests~~ Skipped because all this code is going to be replaced in Implement importable config object #785 soon
Update documentation
Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

…fig-user.yml Please define those settings in the diagnostic script setting of the recipe instead, for those diagnostic scripts that support these settings

bouweandela · 2020-10-02T13:38:24Z

@stefsmeets Do you think it still makes sense to implement unit tests for this, given that you're going to change everything anyway in #785?

stefsmeets

Looks good. I was thinking just marking the config parameters is enough (see comments).

stefsmeets · 2020-10-02T14:24:28Z

esmvalcore/_recipe.py

@@ -1242,15 +1242,22 @@ def _initialize_scripts(self, diagnostic_name, raw_scripts,
            if self._cfg['write_ncl_interface']:
                settings['exit_on_ncl_warning'] = self._cfg['exit_on_warning']
            for key in (
-                    'output_file_type',


I think it is enough to just mark the parameters as deprecated in the config and raise a warning. I think it would be better to keep this part of the code 'as-is' for now. The checks to re-add the keys below adds clutter. Save these for a later patch, and apply it to remove the parameters from the entire codebase for 2.3.

The problem with keeping this as is, is that the parameters from config-user.yml then overwrite what it says in the recipe, and people might get confused that it doesn't work if they add the setting in the recipe

But if the warning is given for the config, then that should be enough in my opinion. Maybe it would be then a good idea to do a double check in the recipe runner and re-issue the warning there.

Maybe it would be then a good idea to do a double check in the recipe runner and re-issue the warning there.

Do you mean here by the 'recipe runner'? Wouldn't that result in the same amount of code clutter, but less user friendly?

Well, I was thinking if that is the problem, we can also warn users as late as possible -- when the recipe is being executed by what I called the recipe runner 😅

I did not understand that the variables from the user config overwrite the recipe, which I guess is what added the complexity in the first place. Whatever is added here, we should make sure it is easy to undo.

stefsmeets · 2020-10-02T14:26:33Z

esmvalcore/_task.py

@@ -367,10 +367,7 @@ def _write_ncl_settings(self):
            'run_dir',
            'plot_dir',
            'work_dir',
-            'output_file_type',


Same here, don't delete these variables just yet and remove the block to add them back below.

some change is needed here, because otherwise these settings will not be available as variable diag_script_info@write_plots etc, making it impossible to adjust diagnostic scripts to the new situtation before they are removed in 2.3. As an intermediate step, I thought I would make them available as both diag_script_info@write_plots and config_user_info@write_plots and then remove the latter in version 2.3.

stefsmeets · 2020-10-02T14:27:54Z

esmvalcore/_task.py

@@ -549,15 +553,12 @@ def _collect_provenance(self):
            'exit_on_ncl_warning',
            'input_files',
            'log_level',
-            'output_file_type',


That would ignore recording their value in the provenance, so it would break the idea that all diagnostic script settings are recorded in the provenance

esmvalcore/_config.py

stefsmeets · 2020-10-02T14:33:16Z

@stefsmeets Do you think it still makes sense to implement unit tests for this, given that you're going to change everything anyway in #785?

Should be alright. The fact that they are deprecated means we should not be testing for it anyway.

bouweandela · 2020-10-02T14:46:24Z

Looks good. I was thinking just marking the config parameters is enough (see comments).

Thanks for reviewing!

valeriupredoi

am not actually requesting changes, I am just thinking these options are indeed not needed anymore but the functional infrastructure they offer is useful - am thinking for the future when we may get to a point where we use a web based interface that runs diagnostics in the backend and the user gets a nice output that they can immediately use for a paper or presentation etc - the write_netcdf and output_format would be nice to be a tunable button

esmvalcore/_config.py

schlunma · 2020-10-02T16:42:26Z

I vote strongly against removing output_file_type. Especially having the command line option output-file-type to quickly change the file format for plots is really convenient. For example, I often switch between PDF (used for TeX or for seperate files of plots sent to journals) and PNG (used for Word).

I don't have a strong opinion about the other two options (more in favor of removing them), but I would really like to keep output_file_type.

valeriupredoi · 2020-10-02T17:25:01Z

I vote strongly against removing output_file_type. Especially having the command line option output-file-type to quickly change the file format for plots is really convenient. For example, I often switch between PDF (used for TeX or for seperate files of plots sent to journals) and PNG (used for Word).

I don't have a strong opinion about the other two options (more in favor of removing them), but I would really like to keep output_file_type.

albeit never used by me (but then again I don't do science), am glad to see this being used and with a good potential for the future quick-run WPS user. Cheers for commenting @schlunma 🍺

bouweandela · 2020-10-05T08:35:21Z

Thanks for the comments @valeriupredoi and @schlunma! Did you also have a look at issue #93 for the reasoning behind removing these settings?

I vote strongly against removing output_file_type. Especially having the command line option output-file-type to quickly change the file format for plots is really convenient. For example, I often switch between PDF (used for TeX or for seperate files of plots sent to journals) and PNG (used for Word).

@schlunma It would be even more convenient if the diagnostic simply wrote both file formats, because then you wouldn't even need to re-run or change a setting to get the plots in a different the format. Since plotting is not a very time-consuming process, I think the time saving is very minimal. The arguments against having the feature are that it provides a poor user experience because many of the diagnostics do not respect the setting and the user needs to select the right output type before running. Also, it might not be so easy to implement this feature consistently across all diagnostics, for example: I doubt whether the R plotting library supports the same output file types as the Python plotting library or the NCL plotting library. I can add a small plotting function to the ESMValTool diag_scripts/shared function that plots several types by default and use it from the example recipe for demonstration purposes, would that make sense? Do you think .png and .pdf would be enough, or would other file formats also make sense?

am thinking for the future when we may get to a point where we use a web based interface that runs diagnostics in the backend and the user gets a nice output that they can immediately use for a paper or presentation etc - the write_netcdf and output_format would be nice to be a tunable button

@valeriupredoi write_netcdf might reduce the runtime and disk space usage of diagnostics that write very large output files, but the majority of diagnostics write very small netcdf files, because there is only so much information that can go into a plot. Therefore it's much more convenient to simply always write all netcdf files. For diagnostic developers who really want to be able to disable this, we can recommend in the documentation that they implement this setting in the recipe. For a web based interface it would probably be nicest to have the netcdf files, because then you can implement an interactive viewer so that the users of the website can zoom in, etc. Regarding configuring the output file type: it's much simpler to always write all plot types, the extra compute time won't be very much.

schlunma · 2020-10-05T08:57:13Z

@schlunma It would be even more convenient if the diagnostic simply wrote both file formats, because then you wouldn't even need to re-run or change a setting to get the plots in a different the format. Since plotting is not a very time-consuming process, I think the time saving is very minimal. The arguments against having the feature are that it provides a poor user experience because many of the diagnostics do not respect the setting and the user needs to select the right output type before running. Also, it might not be so easy to implement this feature consistently across all diagnostics, for example: I doubt whether the R plotting library supports the same output file types as the Python plotting library or the NCL plotting library. I can add a small plotting function to the ESMValTool diag_scripts/shared function that plots several types by default and use it from the example recipe for demonstration purposes, would that make sense? Do you think .png and .pdf would be enough, or would other file formats also make sense?

Alright, that does make sense for me. A small function that writes .png and .pdf would be sufficient for me. Maybe something like this (to consider matplotlib.pyplot.savefig and matplotlib.figure.Figure.savefig)?

import matplotlib.pyplot as plt

def savefig(basename, cfg, fig=None, ext=None **kwargs):
    if ext is None:
        ext = ['png', 'pdf']
    paths = ...  # get correct paths with basename, ext and cfg
    plot_object = plt if fig is None else fig
    for path in paths:
        plot_object.savefig(path, **kwargs)

Just as a small side note: Plotting might be time-consuming in rare cases, e.g. I have an example where I plot a scatterplot with ~200'000 points. In this case it's also not useful to save the file as '*.pdf', since this has more than 100 MB and takes 10min to open on my local machine. However, I think is a rare exception.

valeriupredoi · 2020-10-05T09:18:01Z

cheers for the debate, guys - always useful to discuss things this way 👍 I agree with you all - I am happy the defaults are back and as for the plots file types I think there's more than two right? - eps, png, jpg, pdf, gif (yeah we should implement a gif, we can make climate change memes 😁 - no but seriously, for looking at moving stuff in plots) - so I think that should be tunable since I don't see the need to output all these out. I reckon it's best kept under the hood now (hence remove the optionality from config) but keep the functionality 🍺

valeriupredoi · 2020-10-05T09:20:37Z

@schlunma

However, I think is a rare exception.

might not be as rare as David Attenborough would say - the Autoassess diags output hundreds of plots 🍺

bouweandela · 2020-12-08T15:22:42Z

This needs to be adjusted to take into account #868 and the feature will now be removed in v2.4, because it didn't make it for the 2.1 release.

stefsmeets · 2020-12-09T08:44:34Z

This needs to be adjusted to take into account #868 and the feature will now be removed in v2.4, because it didn't make it for the 2.1 release.

Do we have a deprecation plan somewhere to keep track of which variables are deprecated / removed in which version? Maybe this can be added in #93?

bouweandela · 2021-01-05T13:00:38Z

Do we have a deprecation plan somewhere

No, there is no big plan for what will be deprecated when, but once something is deprecated this should be written down in the changelog for that release, including a note on when the feature will be removed.

Maybe this can be added in #93?

I updated the issue

jvegreg · 2021-01-26T10:32:20Z

Can you please update these, so we can do it for this release?

jvegreg

You missed a couple of 2.3, but nothing really serious

esmvalcore/_config.py

Co-authored-by: Javier Vegas-Regidor <javier.vegas@bsc.es>

…cate-write-plots-netcdf-type

stefsmeets · 2021-01-27T13:35:48Z

Just wondering if these are still up-to-date:

ESMValCore/esmvalcore/experimental/config/_config_validators.py

Line 253 in 9ac52c6

# deprecate in 2.2.0

I marked them for 2.2, but I think it should be 2.3 or 2.4 now also?

bouweandela · 2021-01-27T14:28:06Z

I think they're fine, deprecate in 2.2 (the upcoming release) and remove in 2.4 (in autumn).

…cate-write-plots-netcdf-type

bouweandela · 2021-01-27T15:46:52Z

After looking through the code of ESMValTool to see what it would take to remove output_file_type from config-user.yml, I think that may be a lot of work. I think it's probably not worth the effort.

bouweandela · 2021-01-27T16:18:44Z

@jvegasbsc @valeriupredoi @stefsmeets Could you have another look at this please to see if anything remains to be done before this can be merged? It would be nice if we could include it in the v2.2 release.

valeriupredoi

looks good, cheers @bouweandela - do we have an issue in ESMValTool about this (to remove the bits in the diags that are deprecated)?

bouweandela · 2021-01-27T16:58:11Z

Good point! See ESMValGroup/ESMValTool#2005

valeriupredoi · 2021-01-27T17:05:06Z

Good point! See ESMValGroup/ESMValTool#2005

noice! I added a "future release" label there because it's cool to have one 😁

jvegreg

This two should point to 2.4, no?

esmvalcore/experimental/config/_config_validators.py

Co-authored-by: Javier Vegas-Regidor <javier.vegas@bsc.es>

done

Deprecate use of write_plots, write_netcdf, output_file_type from con…

9d40f74

…fig-user.yml Please define those settings in the diagnostic script setting of the recipe instead, for those diagnostic scripts that support these settings

bouweandela requested review from stefsmeets and valeriupredoi October 2, 2020 13:32

Add a note in documentation

5d9f66c

bouweandela added 2 commits October 2, 2020 15:44

Fix indentation

0c2dccf

Use DEPRECATED instead of TODO to mark deprecated stuff

ddd6e17

stefsmeets previously requested changes Oct 2, 2020

View reviewed changes

valeriupredoi requested changes Oct 2, 2020

View reviewed changes

esmvalcore/_config.py Show resolved Hide resolved

Restore default values and add warning about use from cli

1a9d6dc

bouweandela mentioned this pull request Oct 14, 2020

Improve the Python example diagnostic and documentation ESMValGroup/ESMValTool#1827

Merged

8 tasks

Increase version in which feature will be removed to v2.4

822afd5

bouweandela added this to the v2.2.0 milestone Dec 8, 2020

bouweandela mentioned this pull request Jan 5, 2021

Too many unnessary settings in config-user.yml #93

Closed

bouweandela added 2 commits January 5, 2021 14:11

Merge branch 'master' into deprecate-write-plots-netcdf-type

6a0a73b

Merge branch 'master' into deprecate-write-plots-netcdf-type

a63c460

bouweandela added the deprecated feature label Jan 5, 2021

Update text to v2.2

053e097

jvegreg requested changes Jan 26, 2021

View reviewed changes

esmvalcore/_config.py Outdated Show resolved Hide resolved

esmvalcore/_config.py Outdated Show resolved Hide resolved

bouweandela and others added 2 commits January 27, 2021 13:27

Correct version in which feature will be removed

0e8dba5

Co-authored-by: Javier Vegas-Regidor <javier.vegas@bsc.es>

Merge branch 'master' of github.com:esmvalgroup/esmvalcore into depre…

9ac52c6

…cate-write-plots-netcdf-type

Merge branch 'master' of github.com:esmvalgroup/esmvalcore into depre…

29a944c

…cate-write-plots-netcdf-type

Do not deprecate output_file_type just yet

e9c00f1

valeriupredoi approved these changes Jan 27, 2021

View reviewed changes

bouweandela mentioned this pull request Jan 27, 2021

Options write_plots and write_netcdf will be removed in ESMValCore v2.4 ESMValGroup/ESMValTool#2005

Closed

bouweandela changed the title ~~Deprecate defining write_plots, write_netcdf, output_file_type in config-user file~~ Deprecate defining write_plots and write_netcdf in config-user file Jan 27, 2021

jvegreg previously requested changes Jan 28, 2021

View reviewed changes

esmvalcore/experimental/config/_config_validators.py Outdated Show resolved Hide resolved

esmvalcore/experimental/config/_config_validators.py Outdated Show resolved Hide resolved

Correct version in which feature will be removed

c8c7f9b

Co-authored-by: Javier Vegas-Regidor <javier.vegas@bsc.es>

bouweandela merged commit 1c9885b into master Jan 29, 2021

bouweandela deleted the deprecate-write-plots-netcdf-type branch January 29, 2021 15:47

zklaus mentioned this pull request Dec 20, 2021

Added option to explicitly not use fx variables in preprocessors #1416

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate defining write_plots and write_netcdf in config-user file #808

Deprecate defining write_plots and write_netcdf in config-user file #808

bouweandela commented Oct 2, 2020 •

edited

bouweandela commented Oct 2, 2020

stefsmeets left a comment

stefsmeets Oct 2, 2020 •

edited

bouweandela Oct 2, 2020

stefsmeets Oct 2, 2020

bouweandela Oct 5, 2020

stefsmeets Oct 9, 2020 •

edited

stefsmeets Oct 2, 2020

bouweandela Oct 2, 2020 •

edited

stefsmeets Oct 2, 2020

bouweandela Oct 2, 2020

stefsmeets commented Oct 2, 2020

bouweandela commented Oct 2, 2020

valeriupredoi left a comment

schlunma commented Oct 2, 2020

valeriupredoi commented Oct 2, 2020

bouweandela commented Oct 5, 2020 •

edited

schlunma commented Oct 5, 2020

valeriupredoi commented Oct 5, 2020

valeriupredoi commented Oct 5, 2020

bouweandela commented Dec 8, 2020

stefsmeets commented Dec 9, 2020 •

edited

bouweandela commented Jan 5, 2021 •

edited

jvegreg commented Jan 26, 2021

jvegreg left a comment

stefsmeets commented Jan 27, 2021

bouweandela commented Jan 27, 2021

bouweandela commented Jan 27, 2021

bouweandela commented Jan 27, 2021

valeriupredoi left a comment

bouweandela commented Jan 27, 2021

valeriupredoi commented Jan 27, 2021

jvegreg left a comment

Deprecate defining write_plots and write_netcdf in config-user file #808

Deprecate defining write_plots and write_netcdf in config-user file #808

Conversation

bouweandela commented Oct 2, 2020 • edited

bouweandela commented Oct 2, 2020

stefsmeets left a comment

Choose a reason for hiding this comment

stefsmeets Oct 2, 2020 • edited

Choose a reason for hiding this comment

bouweandela Oct 2, 2020

Choose a reason for hiding this comment

stefsmeets Oct 2, 2020

Choose a reason for hiding this comment

bouweandela Oct 5, 2020

Choose a reason for hiding this comment

stefsmeets Oct 9, 2020 • edited

Choose a reason for hiding this comment

stefsmeets Oct 2, 2020

Choose a reason for hiding this comment

bouweandela Oct 2, 2020 • edited

Choose a reason for hiding this comment

stefsmeets Oct 2, 2020

Choose a reason for hiding this comment

bouweandela Oct 2, 2020

Choose a reason for hiding this comment

stefsmeets commented Oct 2, 2020

bouweandela commented Oct 2, 2020

valeriupredoi left a comment

Choose a reason for hiding this comment

schlunma commented Oct 2, 2020

valeriupredoi commented Oct 2, 2020

bouweandela commented Oct 5, 2020 • edited

schlunma commented Oct 5, 2020

valeriupredoi commented Oct 5, 2020

valeriupredoi commented Oct 5, 2020

bouweandela commented Dec 8, 2020

stefsmeets commented Dec 9, 2020 • edited

bouweandela commented Jan 5, 2021 • edited

jvegreg commented Jan 26, 2021

jvegreg left a comment

Choose a reason for hiding this comment

stefsmeets commented Jan 27, 2021

bouweandela commented Jan 27, 2021

bouweandela commented Jan 27, 2021

bouweandela commented Jan 27, 2021

valeriupredoi left a comment

Choose a reason for hiding this comment

bouweandela commented Jan 27, 2021

valeriupredoi commented Jan 27, 2021

jvegreg left a comment

Choose a reason for hiding this comment

bouweandela commented Oct 2, 2020 •

edited

stefsmeets Oct 2, 2020 •

edited

stefsmeets Oct 9, 2020 •

edited

bouweandela Oct 2, 2020 •

edited

bouweandela commented Oct 5, 2020 •

edited

stefsmeets commented Dec 9, 2020 •

edited

bouweandela commented Jan 5, 2021 •

edited