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

Fix parsing in diag validation #3364

Merged
merged 43 commits into from
Nov 29, 2023
Merged

Conversation

FranziskaWinterstein
Copy link
Contributor

@FranziskaWinterstein FranziskaWinterstein commented Sep 29, 2023

Description

When using the validation.py diagnostic I ran into problems when using aliases and the parsing of the variable ancestor_2 in function _get_provenance_record.
The code changes make use of the alias of the datasets and now finds the proper files if the alias is set manually.


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


…lmean plots. Will be triggered by 'add_tropopause = true' in the recipe
@FranziskaWinterstein FranziskaWinterstein changed the title Draft: Fix parsing in diag validation Fix parsing in diag validation Nov 9, 2023
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 @FranziskaWinterstein for this PR! I have a couple of small suggetions for the code to simplify it.

Before I test existing recipes: can you describe quickly what is going wrong with apply_supermeans? At first glance it looks like your function is doing exactly the same, so I wonder what the problem is.

esmvaltool/diag_scripts/validation.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/validation.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/validation.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/validation.py Outdated Show resolved Hide resolved
@FranziskaWinterstein
Copy link
Contributor Author

Thank you for taking the time and your code updates are just perfect.

Regarding the apply_supermeans. I think the problem is, that the preprocessor function climate_statistics (in preprocessor/_time.py) somehow changed its output. Because when the cubes ctrl and exper are later given to plot_ctrl_exper and plot_zonal_cubes I receive an error. I attached the log file of a run, where this happens:
log.txt

@schlunma
Copy link
Contributor

You are right, there have been some changes in climate_statistics recently, which lead to a couple of problems (e.g., ESMValGroup/ESMValCore#2223). However, this should be fixed by a PR from 10 Oct (ESMValGroup/ESMValCore#2191). Could you please update your version of ESMValCore and try again please? Since your log talks about the coordinate time_weights I guess you are using an older version, in the new the coordinate is called _time_weights_.

@FranziskaWinterstein
Copy link
Contributor Author

Okay, I updated the Core, but now the preprocessor fails completely (main_log_debug.txt) That is however as far as I can go, today. I will look into it again next week, but maybe you have a suggestion what the problem could be already.

@schlunma
Copy link
Contributor

Yes I do! The error is caused by a new feature of iris. Unfortuntately you have to reinstall your environment to solve this properly (i.e., get the latest iris version). Sorry, there have been quite some changes recently.

@FranziskaWinterstein
Copy link
Contributor Author

@schlunma Yes, you were right. I needed to reinstall conda anyway. With the current ESMValCore release as well as the current development ESMValCore everything went fine with the supermeans function. So I will revoke the changes regarding supermeans.

@FranziskaWinterstein
Copy link
Contributor Author

Ready to merge, I think.

@schlunma
Copy link
Contributor

schlunma commented Nov 28, 2023

I just tested this with the existing recipes recipe_validation.yml and recipe_validation_CMIP6.yml. Unfortunately, both failed with the following error:

Traceback (most recent call last):
  File "/home/b/b309141/repos/ESMValTool/esmvaltool/diag_scripts/validation.py", line 418, in <module>
    main(config)
  File "/home/b/b309141/repos/ESMValTool/esmvaltool/diag_scripts/validation.py", line 383, in main
    plot_ctrl_exper_seasons(ctrl_seasons, exper_seasons, cfg, plot_key)
  File "/home/b/b309141/repos/ESMValTool/esmvaltool/diag_scripts/validation.py", line 353, in plot_ctrl_exper_seasons
    plot_zonal_cubes(control_season, experiment_season, cfg, plot_info)
  File "/home/b/b309141/repos/ESMValTool/esmvaltool/diag_scripts/validation.py", line 223, in plot_zonal_cubes
    save_plotted_cubes(
  File "/home/b/b309141/repos/ESMValTool/esmvaltool/diag_scripts/validation.py", line 103, in save_plotted_cubes
    _get_provenance_record(cfg, save_path,
  File "/home/b/b309141/repos/ESMValTool/esmvaltool/diag_scripts/validation.py", line 43, in _get_provenance_record
    ancestor_1 = [
                 ^
IndexError: list index out of range

The reason for this is that the code that retrieves the ancestors is really not optimal. It uses lots of split('_'), which fails if aliases look different (they may or may not contain _). I tried to fix this in 90fc8dc, but this is still not really nice. This diagnostic would need a lot of changes to make it more robust (for example, explicitly pass aliases and ancestors to the functions instead of relying on weird strings that somehow include this information).

@FranziskaWinterstein Please let me know if this also works for your use case; our two validation recipes run successfully and produce consistent resuls 👍

@FranziskaWinterstein
Copy link
Contributor Author

@schlunma Yes, my recipe output is identical.

I was also confused that the names are retrieved from the output filenames, which fails easily, if something changes there.

@schlunma schlunma added this to the v2.11.0 milestone Nov 29, 2023
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.

Great, then I think this can be merged!

@valeriupredoi, as the author of this diagnostic, could you please do a final check and merge? Thanks!!

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.

looking good, many thanks! 🍺

@valeriupredoi
Copy link
Contributor

guys, bear in mind this diagnostic predates most all the bits that you mention, it's ye olde, but still going strong if you don't need to do fancy stuff 😁

@esmvalbot please run recipe_validation.yml

@valeriupredoi
Copy link
Contributor

@esmvalbot please run recipe_validation.yml

@esmvalbot
Copy link

esmvalbot bot commented Nov 29, 2023

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

@esmvalbot
Copy link

esmvalbot bot commented Nov 29, 2023

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

@valeriupredoi valeriupredoi merged commit 5ba10a5 into main Nov 29, 2023
6 checks passed
@valeriupredoi valeriupredoi deleted the fix_parsing_in_diag_validation branch November 29, 2023 16:25
jvegreg pushed a commit that referenced this pull request Jan 14, 2024
Co-authored-by: Axel Lauer <[email protected]>
Co-authored-by: Manuel Schlund <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: valeriupredoi <[email protected]>
Co-authored-by: Valeriu Predoi <[email protected]>
Co-authored-by: Manuel Schlund <[email protected]>
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.

4 participants