re-add correctly region-extracted cell measures and ancillary variables after `extract_region` (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

valeriupredoi · 2023-08-07T12:52:13Z

Description

Cube intersection removes all cell measures and ancillary variables in certain conditions (for certain regions ie certain coordinates of the region box). This has been raised upstream, see below. NOTE for Codacy fails: I intentionally placed the imports inside block for ease of removal when we will eventually remove the workaround code block,

Closes #2162

Brought to my attention by @rebeccaherman1 in the above linked issue, which is, in fact a bug in iris - raised upstream in SciTools/iris#5413 - this is a workaround (temporary too) so that we have something outputting correct data before iris fix it at their end.

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 pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

🧪 The new functionality is relevant and scientifically sound
🛠 This pull request has a descriptive title and labels
🛠 Code is written according to the code quality guidelines
🛠 Unit tests have been added
🛠 All checks below this pull request were successful

To help with the number pull requests:

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

codecov · 2023-08-07T13:34:46Z

Codecov Report

Merging #2166 (345d7bf) into main (ea29584) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2166      +/-   ##
==========================================
+ Coverage   93.09%   93.10%   +0.01%     
==========================================
  Files         237      237              
  Lines       12795    12816      +21     
==========================================
+ Hits        11911    11932      +21     
  Misses        884      884

Files Changed	Coverage Δ
esmvalcore/preprocessor/_area.py	`93.65% <100.00%> (+0.42%)`	⬆️

valeriupredoi · 2023-08-07T14:54:21Z

BTW @ledm I have a feeling that this bit you as well, in your continual battle with fx variables

schlunma

Thanks V for tackling this!

I have three main comments:

I think you can simplify this by re-using extract_region on a cell measure/ancillary variable cube (see code suggestions).
Why do change the formatting of unrelated code? This has been formatted by black to look nice and shiny ⭐
Could you maybe come up with a test that doesn't require adding a 1.1 MB file to the repo? We have plenty of example code that sets up a cube with a CellMeasure, e.g., here.

esmvalcore/preprocessor/_area.py

schlunma · 2023-08-07T15:36:36Z

esmvalcore/preprocessor/_area.py

@@ -467,15 +531,13 @@ def _process_ids(geometries, ids: list | dict | None) -> tuple:
        if len(ids) != 1:
            raise ValueError(
                f"If `ids` is given as dict, it needs exactly one entry, got "
-                f"{ids}"
-            )
+                f"{ids}")


Why do you change the nice formatting by black? 😝

ah yes, totally my bad, I yapf-ed it, flake8 was getting on me nerves - but, we don't use black so why the bother?

if we use it for precommit stuff, I'd turn it off TBF, black is black magic and introduces a lot of shtuff on its own

my bad for yapf-ing it, should I restore the old formatting from unrelated sections?

Yeah, would be nice to change it back 😁

Sounds good, bud, shall revert, in a jiffy

will tacke the other suggestions that I got from you (git them in already, but need to make sure all's shipshape, fix the tests too) then will backtrack on this, 🐻 with me for a few, bud

valeriupredoi · 2023-08-07T16:14:45Z

Cheers muchly, Manu! 🍺

I think you can simplify this by re-using extract_region on a cell measure/ancillary variable cube (see code suggestions).

I'm a bit afraid of unexpected behaviour knowing that cube.intersect is very moody - maybe let's fix it if it's indeed broken

Why do change the formatting of unrelated code? This has been formatted by black to look nice and shiny star

my bad for yapf-ing it, should I restore the old formatting from unrelated sections?

Could you maybe come up with a test that doesn't require adding a 1.1 MB file to the repo? We have plenty of example code that sets up a cube with a CellMeasure, e.g., here.

Indeed, I can do that - I was lazy and knew this will be removed at some point in the future anyway 😁

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

esmvalcore/preprocessor/_area.py

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

valeriupredoi · 2023-08-08T13:04:38Z

with the latest changes when you try print the cube that's been changed you get this:

Traceback (most recent call last):
  File "/home/valeriu/test_intersect.py", line 27, in <module>
    print("ex1", ex1)
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/cube.py", line 2563, in __str__
    return self.summary()
           ^^^^^^^^^^^^^^
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/cube.py", line 2558, in summary
    printer = CubePrinter(self)
              ^^^^^^^^^^^^^^^^^
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/_representation/cube_printout.py", line 165, in __init__
    cube_summary = CubeSummary(cube_or_summary)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/_representation/cube_summary.py", line 364, in __init__
    add_vector_section(
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/_representation/cube_summary.py", line 355, in add_vector_section
    self.vector_sections[title] = VectorSection(
                                  ^^^^^^^^^^^^^^
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/_representation/cube_summary.py", line 203, in __init__
    self.contents = [
                    ^
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/_representation/cube_summary.py", line 204, in <listcomp>
    VectorSummary(cube, vector, iscoord) for vector in vectors
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/valeriu/miniconda3/envs/esmvaltool/lib/python3.11/site-packages/iris/_representation/cube_summary.py", line 141, in __init__
    dims = vector.cube_dims(cube)
           ^^^^^^^^^^^^^^^^
AttributeError: 'Cube' object has no attribute 'cube_dims'

This reverts commit 3f0dede.

This reverts commit 85e8b33.

This reverts commit 429a0c7.

This reverts commit 6d8652a.

This reverts commit 15d4973.

valeriupredoi · 2023-08-08T13:30:40Z

sorry @schlunma a few too many issues with your review suggestions (both at our end and iris - iris issue which I am not prepared to dive into at the moment, see #2166 (comment)); if you think we'd benefit enough from those changes, I suggest we merge this, then you can open a PR based off main where you apply those changes, and test properly. I'll fix the test not to use the test data (and delete that file), then I'll revert the style changes resulted from yapfing it, then I am ready to merge this 🍺

valeriupredoi · 2023-08-08T14:31:22Z

OK Manu, I have finished poslishing this:

used your suggestion to allow for irregular grids to be looked at
fixed the test and removed the now unnecessary test data file
re-blacked _area.py (well, reverted previously added style changes from yapf)

I think we should do this: merge this to have correct functionality for extract_region, then you and me we can go back to the suggestions you made earlier in a follow up PR, unfortunately there are quite a few hitches (not least the iris one that we should look at) to be looked at here; we may as well not even do it all if we'll remove all this when iris fix their intersection, but at least I'd like to understand that print(cube) issue above. What say you? 🍺

valeriupredoi · 2023-08-08T14:36:39Z

Also, as a note to all of us (I am guilty as charged myself) - everytime we suggest major code changes in a PR, in the form of suggested code via the browser/GitHub API, it would be a lot easier for the PR owner if the reviewer tested those changes by either running the test provided in the PR or by running a small independent test - this, to catch serious issues (not style, minor issues with the suggestions, those can be taken care of the PR owner, after including the suggested code changes). I think we should add this to our best practices PR reviewing docs 👍

schlunma · 2023-08-08T14:44:16Z

Well, if you use cube.intersection (and not extract_region) to get the cell measure sub cube, this won't work for irregular grids. Hence you need to reinstate the check for coord('latitude').ndim == 1.

Also, in this form, this won't work (and will actually crash) for cell measures and ancillary variables that do not exactly span latitude/longitude. We need to take care of this properly here, otherwise it will break recipes.

I am honestly confused why my suggestion didn't work. From the failing test on CircleCI it looks just like a flake8 fail? I will give it a try myself in a local branch.

valeriupredoi · 2023-08-08T14:52:07Z

Well, if you use cube.intersection (and not extract_region) to get the cell measure sub cube, this won't work for irregular grids. Hence you need to reinstate the check for coord('latitude').ndim == 1.

Also, in this form, this won't work (and will actually crash) for cell measures and ancillary variables that do not exactly span latitude/longitude. We need to take care of this properly here, otherwise it will break recipes.

I am honestly confused why my suggestion didn't work. From the failing test on CircleCI it looks just like a flake8 fail? I will give it a try myself in a local branch.

sounds like a very good plan! Cheers, Manu! Can grab this branch, edit locally, and when things look OK to you just push changes here - that was my point in #2166 (comment) - reviewer's minimal testing is something we should start doing 🍺

schlunma · 2023-08-08T14:52:33Z

Tests pass fine for me locally (your test and my suggestion). Can I push it? 😁

valeriupredoi · 2023-08-08T14:54:31Z

Tests pass fine for me locally (your test and my suggestion). Can I push it? grin

Pliz do 🍺

…y dimensionality

valeriupredoi · 2023-08-08T15:47:53Z

That is now beautiful, and I don't see any more iris-related issues either - excellent work, Manu! 🍻

valeriupredoi · 2023-08-08T16:09:05Z

@schlunma let's get this in plz so I can help @rebeccaherman1 with an install at DKRZ 🍺

schlunma

Thanks V! Can I be a bother and ask to to test this with the script mentioned here? I think we need to make sure that this properly works for cubes with time dimensions, too (If you like, you could also add a brief test of this 😁).

I will be in a seminar today with limitied availability, so I pre-approve this. Thanks for the fix!!

valeriupredoi · 2023-08-09T08:53:26Z

Thanks V! Can I be a bother and ask to to test this with the script mentioned here? I think we need to make sure that this properly works for cubes with time dimensions, too (If you like, you could also add a brief test of this grin).

I will be in a seminar today with limitied availability, so I pre-approve this. Thanks for the fix!!

Very many thanks, Manu! Yes, that's what I've been using to test the PR (alas, slightly changed, but I'll test with the exact script that Rebecca had initially). Good point, I'll put a time dim in the integration test in this PR 🍺

valeriupredoi · 2023-08-09T13:26:02Z

Boss level, Manu! Added time dim and check on final shapes, think the test is pretty bullet-proof now. Gonna merge and get @rebeccaherman1 an env done on DKRZ so she can get her data done up 👍

readd cell measure after extract region

a1f9401

valeriupredoi added the bug Something isn't working label Aug 7, 2023

valeriupredoi requested review from schlunma and sloosvel August 7, 2023 12:52

valeriupredoi added 5 commits August 7, 2023 14:01

add conditionals

5df45b3

readd ancillary variable

41583f6

fix flake

a7e9d65

yapf the crap outta it

eaf39d5

can flake pls shut up

2e8c8b4

valeriupredoi added 4 commits August 7, 2023 14:56

add test case dir

2352688

appease flake

eeca3d9

add test netcdf file

85ec670

polish inline comments

93b0858

valeriupredoi added this to the v2.10.0 milestone Aug 7, 2023

valeriupredoi mentioned this pull request Aug 7, 2023

supplementary variables disappear from cubes after extract_region #2162

Closed

valeriupredoi changed the title ~~re-add correctly region-extracted cell measures after extract_region~~ re-add correctly region-extracted cell measures and ancillary variables after extract_region Aug 7, 2023

schlunma requested changes Aug 7, 2023

View reviewed changes

valeriupredoi and others added 6 commits August 8, 2023 12:39

Manu suggestion

15d4973

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

Manu suggestion to deal with irregular grids too

6d8652a

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

fix flake

429a0c7

fix booboo from Manu

85e8b33

plug in Manu's infinite recursion to fix it in terminal

3f0dede

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

unfubar the workflow

2efbc9e

schlunma reviewed Aug 8, 2023

View reviewed changes

esmvalcore/preprocessor/_area.py Outdated Show resolved Hide resolved

esmvalcore/preprocessor/_area.py Outdated Show resolved Hide resolved

valeriupredoi and others added 2 commits August 8, 2023 14:02

Update esmvalcore/preprocessor/_area.py

74b979b

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

Update esmvalcore/preprocessor/_area.py

bf35614

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>

valeriupredoi added 6 commits August 8, 2023 14:21

Revert "plug in Manu's infinite recursion to fix it in terminal"

107c9e0

This reverts commit 3f0dede.

Revert "fix booboo from Manu"

619ebb1

This reverts commit 85e8b33.

Revert "fix flake"

7757f36

This reverts commit 429a0c7.

Revert "Manu suggestion to deal with irregular grids too"

34707ad

This reverts commit 6d8652a.

Revert "Manu suggestion"

73b2b0c

This reverts commit 15d4973.

apply Manu suggestion to generalize for irreg grids too

78cb1d3

valeriupredoi added 4 commits August 8, 2023 14:51

re-black code

c1825ec

missed a couple blacks

6964fec

optimized test without need of data file

e085a5e

rm data file

24114bd

valeriupredoi and others added 2 commits August 8, 2023 16:20

Merge branch 'main' into readd_cell_measure_after_extract_region

bdb88ad

Made extract_region work with cell measures and ancillaries with ever…

2420fba

…y dimensionality

valeriupredoi changed the title ~~re-add correctly region-extracted cell measures and ancillary variables after extract_region~~ re-add correctly region-extracted cell measures and ancillary variables after extract_region (for Changelog v2.10: authors: @valeriupredoi and @schlunma) Aug 8, 2023

valeriupredoi mentioned this pull request Aug 8, 2023

Add two authors in release Changelog (per PR) if contributions warrant it #2167

Open

schlunma approved these changes Aug 9, 2023

View reviewed changes

add time dim to test cube

0bb1101

fix indentation

345d7bf

valeriupredoi merged commit facd794 into main Aug 9, 2023
4 checks passed

valeriupredoi deleted the readd_cell_measure_after_extract_region branch August 9, 2023 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-add correctly region-extracted cell measures and ancillary variables after `extract_region` (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

re-add correctly region-extracted cell measures and ancillary variables after `extract_region` (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

valeriupredoi commented Aug 7, 2023 •

edited

codecov bot commented Aug 7, 2023 •

edited

valeriupredoi commented Aug 7, 2023

schlunma left a comment •

edited

schlunma Aug 7, 2023

valeriupredoi Aug 7, 2023

valeriupredoi Aug 7, 2023

schlunma Aug 8, 2023

valeriupredoi Aug 8, 2023

valeriupredoi Aug 8, 2023

valeriupredoi commented Aug 7, 2023 •

edited

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

schlunma commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

schlunma commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

schlunma left a comment

valeriupredoi commented Aug 9, 2023

valeriupredoi commented Aug 9, 2023

re-add correctly region-extracted cell measures and ancillary variables after extract_region (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

re-add correctly region-extracted cell measures and ancillary variables after extract_region (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

Conversation

valeriupredoi commented Aug 7, 2023 • edited

Description

codecov bot commented Aug 7, 2023 • edited

Codecov Report

valeriupredoi commented Aug 7, 2023

schlunma left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriupredoi commented Aug 7, 2023 • edited

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

schlunma commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

schlunma commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

valeriupredoi commented Aug 8, 2023

schlunma left a comment

Choose a reason for hiding this comment

valeriupredoi commented Aug 9, 2023

valeriupredoi commented Aug 9, 2023

re-add correctly region-extracted cell measures and ancillary variables after `extract_region` (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

re-add correctly region-extracted cell measures and ancillary variables after `extract_region` (for Changelog v2.10: authors: @valeriupredoi and @schlunma) #2166

valeriupredoi commented Aug 7, 2023 •

edited

codecov bot commented Aug 7, 2023 •

edited

schlunma left a comment •

edited

valeriupredoi commented Aug 7, 2023 •

edited