Make it possible to use write_plots and write_netcdf from recipe instead of config-user.yml

[bouweandela]

Description

Make it possible to use write_plots and write_netcdf from recipe instead of config-user.yml and update the Python example recipe so it does this.

• Related to Options write_plots and write_netcdf will be removed in ESMValCore v2.4 #2005

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

Checklist

It is the responsibility of the author to make sure the PR is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

• 🛠 PR has a descriptive title for the changelog
• 🛠 Code follows the style guide
• 🛠 Documentation is available
• 🛠 YAML files pass pre-commit or yamllint checks
• 🛠 Circle/CI tests pass
• 🛠 Codacy code quality checks pass
• 🛠 Documentation builds successfully on readthedocs

New or updated recipe/diagnostic:

• 🧪 Documentation for the recipe/diagnostic clearly describes what it calculates from a scientific point of view
• 🧪 Recipe runs successfully on @esmvalbot or some other machine without modification
• 🧪 Figure(s)/data look as expected

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[bouweandela]

@esmvalbot Please run examples/recipe_python.yml

[esmvalbot]

Since @bouweandela asked, ESMValBot will run recipe examples/recipe_python.yml as soon as possible, output will be generated here

[esmvalbot]

ESMValBot is happy to report recipe examples/recipe_python.yml ran OK, output has been generated here

[bettina-gier]

Following the conversations in the other related pull requests and issues - shouldn't it be a default to write netcdf/plot files, so add it more as an optional argument to turn it off ("no_plots"/"no_netcdf"), rather than on? This would feel more in line with the diagnostics that currently don't have the on/off capabilities.

I don't think changing the example recipe that new developers might look at to include an option to turn it on when it should be on by default is a good idea. Might also just be me nitpicking though ^^

[schlunma]

@esmvalbot Please run recipe_seaice.yml

[esmvalbot]

Since @schlunma asked, ESMValBot will run recipe recipe_seaice.yml as soon as possible, output will be generated here

[esmvalbot]

ESMValBot is sorry to report it failed to run recipe recipe_seaice.yml: exit is 1, output has been generated here

[schlunma]

Some general comments in addition to the comments on the code:

• I agree that write_netcdf and write_plots might not be optimal names, but since some python diagnostics are using them we should stick to it for consistency.
• I agree with @bettina-gier that we should not use write_netcdf in an example recipe if we think that user's should not use it.
• The new optional attributes for all affected diagnostics need to documented in the docstring/header of the diagnostic and in the rst file of the corresponding recipe.
• We should make sure that all python diagnostics still run when write_plots and write_netcdf are not added to cfg automatically, e.g. by making sure to use cfg.get('write_plots', True) instead of cfg['write_plots'].
• I think we should not use custom diagnostic options in code that affects all diagnostics, i.e. remove this check here:

  ESMValTool/esmvaltool/diag_scripts/shared/_base.py

  Line 130 in 4034661

  if cfg.get('write_netcdf', True):

[bouweandela]

Thanks for your feedback @bettina-gier and @schlunma.

Following the conversations in the other related pull requests and issues - shouldn't it be a default to write netcdf/plot files, so add it more as an optional argument to turn it off ("no_plots"/"no_netcdf"), rather than on? This would feel more in line with the diagnostics that currently don't have the on/off capabilities.
I don't think changing the example recipe that new developers might look at to include an option to turn it on when it should be on by default is a good idea.

I agree that write_netcdf and write_plots might not be optimal names, but since some python diagnostics are using them we should stick to it for consistency.

That latter remark was also my thinking in keeping the name write_netcdf. It's not just Python diagnostics, it's also quite a few NCL and R diagnostics. Note that what is deprecated is setting this function in the config-user.yml file, diagnostics developers are free to add whatever options they feel are suitable to their diagnostics. In particular, some authors feel that writing NetCDF output at all is a bad idea #727 (comment). I would like to encourage writing NetCDF output, because I think it would be really useful if at some point we want to present the output with interactive plotting functionality on a website and it will also make it more easy to implement regression tests for recipes. My thought in adding it to the example was that those people might also be convinced to add it to their diagnostics, but maybe you're right and it's too complicated for a first example, I removed it for now.

• The new optional attributes for all affected diagnostics need to documented in the docstring/header of the diagnostic and in the rst file of the corresponding recipe.
• We should make sure that all python diagnostics still run when write_plots and write_netcdf are not added to cfg automatically, e.g. by making sure to use cfg.get('write_plots', True) instead of cfg['write_plots'].
• Otherwise this will fail in version 2.4 when the write_plots attribute is not added to diag_script_info automatically anymore. Applies to all similar changes

@schlunma I would prefer to leave this for now. I think it is up to the recipe maintainers to update their recipes and associated documentation before the feature is removed. I just made the changes to the .ncl files so it's actually possible to run the recipe by just updating that, without the need to change code. Would that be acceptable for you?

[schlunma]

Thanks for the changes!

@schlunma I would prefer to leave this for now. I think it is up to the recipe maintainers to update their recipes and associated documentation before the feature is removed. I just made the changes to the .ncl files so it's actually possible to run the recipe by just updating that, without the need to change code. Would that be acceptable for you?

This would of course be the optimal solution, but in practice this is most likely not feasible. All of the affected recipes/diagnostics will fail in v2.4 and one person (probably the poor release manager) will have to fix the majority of them. Moreover, I fear that diagnostic developers will use the existing diagnostics as templates for developing new ones, which will create more diagnostics/recipes that need to be changed in the future.

Do we have a proper documentation for the deprecation (an issue and maybe also in the documentation itself)? If yes, I can live with that, but I'm not super happy with it.

Can you please remove the write_netcdf option here

ESMValTool/esmvaltool/diag_scripts/shared/_base.py

Line 130 in ca6d17d

if cfg.get('write_netcdf', True):

? I don't like this hidden condition on diagnostic specific variables. I think if this feature is desired it should be used in the diagnostic like this:

if cfg.get('write_netcdf', True):
    save_data(...)

[bouweandela]

Do we have a proper documentation for the deprecation (an issue and maybe also in the documentation itself)?

Yes, we have issue #2005, a mention in the ESMValCore changelog and it is also mentioned in the documentation on config-user.yml here, though for some reason the documentation build looks outdated.

Moreover, I fear that diagnostic developers will use the existing diagnostics as templates for developing new ones, which will create more diagnostics/recipes that need to be changed in the future.

There is the documentation that describes the feature as deprecated and technical reviewers should be aware of this too.

Can you please remove the write_netcdf option here

ESMValTool/esmvaltool/diag_scripts/shared/_base.py

Will do

[schlunma]

@esmvalbot Please run examples/recipe_python.yml

[esmvalbot]

Since @schlunma asked, ESMValBot will run recipe examples/recipe_python.yml as soon as possible, output will be generated here

[esmvalbot]

ESMValBot is happy to report recipe examples/recipe_python.yml ran OK, output has been generated here

[schlunma]

Great! Ready to merge now 🎉

[bouweandela]

Thanks for reviewing!