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.

Sign up for GitHub

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

Jump to bottom

Use transform_first=True for contourf plots with Robinson projection to avoid cartopy bug #3789

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Use transform_first=True for contourf plots with Robinson projection to avoid cartopy bug #3789

merged 3 commits into from
Oct 30, 2024

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Oct 22, 2024 •
edited
Loading

Description

See SciTools/cartopy#2457: using the Robinson projection with contourf may lead to very weird and wrong plots. This PR fixes that by setting transform_first=True as suggested here.

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.12.0 milestone Oct 22, 2024
@schlunma schlunma self-assigned this Oct 22, 2024
@schlunma schlunma changed the title Remove central_longitude=10.0 for cartopy.crs.Robinson projection plots to avoid cartopy bug Use transform_first=True for contourf plots with Robinson projection to avoid cartopy bug Oct 29, 2024
@schlunma schlunma marked this pull request as ready for review October 29, 2024 16:39
@schlunma schlunma requested a review from a team as a code owner October 29, 2024 16:39
@schlunma
Copy link
Contributor Author

@esmvalbot please run monitor/recipe_monitor_with_refs.yml

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2024

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

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2024

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

bettina-gier
bettina-gier approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@bettina-gier bettina-gier left a comment

Choose a reason for hiding this comment

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

Recipe runs fine, output looks good.

Only issue is, are these the only affected diagnostics? contourfplots are quite common.

@valeriupredoi
Copy link
Contributor

Bot says recipe monitor runs fine too #3791 (comment) - figured out what the bot's problem was (in private on Slack 🤣 )

@valeriupredoi
Copy link
Contributor

@esmvalbot please run monitor/recipe_monitor_with_refs.yml

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2024

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

@esmvalbot
Copy link

esmvalbot bot commented Oct 29, 2024

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

@valeriupredoi
Copy link
Contributor

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

you smol bundle of joy of a bot 🤖

valeriupredoi
valeriupredoi approved these changes Oct 29, 2024
View reviewed changes
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.

thanks Manu! Nice PR, but that Bot running your recipe - now that's something 😁 There are explicit calls to iris.plot.contourf in esmvaltool/diag_scripts/ocean but I don't know if those are Robinson projections; there are heaploads of Robinson projections being used in many other diags, that use plotting, but am not sure if those are iris plots - I think we need a thorough check, or we wait to see if results are crap, or anything inbetween these two strategies. I am not sure what's best

@schlunma
Copy link
Contributor Author

Good points! AFAIK, this is only relevant for map contouf plots (e.g., not for zonal mean profiles). I found the bug for the diagnostics that I changed here, but these are probably not the only ones affected. However, without deeper knowledge of the individual diagnostics it's really not easy to find out which are affected and which not.

My suggestion would be to either merge this now and be aware of that issue, or add this keyword to all mentions of contourf in our code (not sure if that might lead to other problems though...).

@bettina-gier
Copy link
Contributor

I agree on not changing diagnostics we don't really know about and merging this pull request first for an immediate fix.

However, this isn't the only time something like this happens, would it be a good idea to include an "errata" page, maybe in the docs, where, potentially grouped by releases, we collect these 'found this error but not sure if anything else is affected' issues are collected. Then during release testing we can link to that page for diagnostic maintainers as a first step to check if that is one of the issues when their recipe fails. I've often seen people during the release testing saying "hey this issue was already reported here insert link", when it might be good to just have a list of it to begin with.

@schlunma
Copy link
Contributor Author

In principle I like this idea, but this will be hard to maintain. It will be very easy to forget to add things, remove outdated things, etc.

In a way this is already documented in this PR here, right? So if people search the repository before reporting new problems (which people should always do!), this will be found. I am gonna merge this now to fix the diags; but we can of course further discuss this.

@schlunma schlunma merged commit 0961c45 into main Oct 30, 2024
8 checks passed
@schlunma schlunma deleted the remove_central_longitude branch October 30, 2024 11:16
@bettina-gier
Copy link
Contributor

bettina-gier commented Oct 30, 2024 •
edited
Loading

hmm that's why I wanted it in sections of "found after release x before release y". Maybe docs is a bad place, a pinned issue which links to the issues with a disclaimer of not being updated beyond adding the links

Edit: maybe a point for the workshop as "dissemination of information"

@schlunma schlunma mentioned this pull request Oct 31, 2024
Merged
12 tasks
@schlunma
Copy link
Contributor Author

Found the first problem with this: SciTools/cartopy#2468 🙈

Luckily, the fix is easy: #3797

ehogan added a commit that referenced this pull request Nov 19, 2024
@ehogan
Merge branch 'main' into recipe_test_workflow_prototype
09490de 
* main: (31 commits)
  Update environment: pin `iris>=3.11`, unpin `cartopy` and allow for `numpy >=2` (#3811)
  Fix issue related to removal/change of private function imported in `diag_scripts/shared/_supermeans.py` (deprecation in iris=3.11) (#3810)
  Remove recipe filler utility (#3777)
  [Condalock] Update Linux condalock file (#3809)
  change authors name (#3806)
  [Condalock] Update Linux condalock file (#3798)
  Fix contourf plots for masked data (#3797)
  [Condalock] Update Linux condalock file (#3796)
  Add next release schedule (#3794)
  Use `transform_first=True` for contourf plots with Robinson projection to avoid cartopy bug (#3789)
  Pin pys2index >=0.1.5 in osx environment (#3792)
  Adding a CMORiser for CMAP data for pr (#3766)
  Adding pr, tauu, tauv, tos to NCEP2 CMORISer (#3765)
  Readthedocs configuration/builds: revert to miniconda before miniforge is available (#3785)
  Adapt ESMValTool to new configuration (#3761)
  Adding pr, tauu, tauv NOAA-CIRES-20CR-V2 CMORISER (#3763)
  update comment in conda lock creation Github action (#3788)
  [Condalock] Update Linux condalock file (#3786)
  update Docker builds badge in README (#3783)
  Pin mamba<2 for conda-lock: solution by Ben Mares @maresb (#3771)
  ...
ehogan added a commit that referenced this pull request Nov 19, 2024
@ehogan
Merge branch '3723_remove_rose_cylc' of github.com:ESMValGroup/ESMVal…
90c8ca1 
…Tool into 3723_remove_rose_cylc

* '3723_remove_rose_cylc' of github.com:ESMValGroup/ESMValTool: (31 commits)
  Update environment: pin `iris>=3.11`, unpin `cartopy` and allow for `numpy >=2` (#3811)
  Fix issue related to removal/change of private function imported in `diag_scripts/shared/_supermeans.py` (deprecation in iris=3.11) (#3810)
  Remove recipe filler utility (#3777)
  [Condalock] Update Linux condalock file (#3809)
  change authors name (#3806)
  [Condalock] Update Linux condalock file (#3798)
  Fix contourf plots for masked data (#3797)
  [Condalock] Update Linux condalock file (#3796)
  Add next release schedule (#3794)
  Use `transform_first=True` for contourf plots with Robinson projection to avoid cartopy bug (#3789)
  Pin pys2index >=0.1.5 in osx environment (#3792)
  Adding a CMORiser for CMAP data for pr (#3766)
  Adding pr, tauu, tauv, tos to NCEP2 CMORISer (#3765)
  Readthedocs configuration/builds: revert to miniconda before miniforge is available (#3785)
  Adapt ESMValTool to new configuration (#3761)
  Adding pr, tauu, tauv NOAA-CIRES-20CR-V2 CMORISER (#3763)
  update comment in conda lock creation Github action (#3788)
  [Condalock] Update Linux condalock file (#3786)
  update Docker builds badge in README (#3783)
  Pin mamba<2 for conda-lock: solution by Ben Mares @maresb (#3771)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valeriupredoi valeriupredoi valeriupredoi approved these changes

@bettina-gier bettina-gier bettina-gier approved these changes

Assignees

@schlunma schlunma

Labels
diagnostic
Projects
None yet
Milestone
v2.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@schlunma @valeriupredoi @bettina-gier