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

Can't collapse over a mesh dimension #4672

Closed
pp-mo opened this issue Mar 29, 2022 · 7 comments · Fixed by #4870
Closed

Can't collapse over a mesh dimension #4672

pp-mo opened this issue Mar 29, 2022 · 7 comments · Fixed by #4870

Comments

@pp-mo
Copy link
Member

pp-mo commented Mar 29, 2022

A cube cannot currently be collapsed over a MeshCoord (or mesh dim),
since the collapse operation expects a bounds shape of (N, 2) and it can't handle (N, 4).

Example:

>>> import iris.tests.stock.mesh as ism
>>> cube = ism.sample_mesh_cube()
>>> cube.collapsed('latitude', iris.analysis.MEAN)
/home/h05/itpp/git/iris/iris_main/lib/iris/cube.py:3696: UserWarning: Collapsing spatial coordinate 'latitude' without weighting
  warnings.warn(msg.format(coord.name()))
/home/h05/itpp/git/iris/iris_main/lib/iris/coords.py:2220: UserWarning: Collapsing a non-contiguous coordinate. Metadata may not be fully descriptive for 'i_mesh_face'.
  warnings.warn(msg.format(self.name()))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/cube.py", line 3744, in collapsed
    collapsed_cube.replace_coord(coord.collapsed(local_dims))
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/coords.py", line 2215, in collapsed
    elif not self.is_contiguous():
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/coords.py", line 2018, in is_contiguous
    contiguous, _ = self._discontiguity_in_bounds(rtol=rtol, atol=atol)
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/coords.py", line 1944, in _discontiguity_in_bounds
    self._sanity_check_bounds()
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/coords.py", line 1900, in _sanity_check_bounds
    raise ValueError(
ValueError: Invalid operation for 'latitude', with 4 bound(s). Contiguous bounds are only defined for 1D coordinates with 2 bounds.
>>> 

So, the problem isn't really specific to the peculiarities of MeshCoords, it is about the shape of the bounds :
(N. 2) is expected and nothing else works.
However, it would make perfect sense to discard the bounds here and proceed (with a warning).

In the case of a MeshCoord, we can see that this does produce a perfectly sensible result.

>>> cube = cube[..., 0:]
>>> print(cube)
mesh_phenom / (unknown)             (level: 2; i_mesh_face: 3)
    Dimension coordinates:
        level                             x               -
        i_mesh_face                       -               x
    Auxiliary coordinates:
        latitude                          -               x
        longitude                         -               x
        mesh_face_aux                     -               x
>>> cube.collapsed('latitude', iris.analysis.MEAN)
   . . .
ValueError: Invalid operation for 'latitude', with 4 bound(s). Contiguous bounds are only defined for 1D coordinates with 2 bounds.
>>> 
>>> cube.coord('latitude').bounds = None
>>> cube.coord('longitude').bounds = None
>>> cube_1d = cube.collapsed('latitude', iris.analysis.MEAN)
>>> print(cube_1d)
mesh_phenom / (unknown)             (level: 2)
    Dimension coordinates:
        level                             x
    Scalar coordinates:
        i_mesh_face                 1, bound=(0, 2)
        latitude                    3201, bound=(3200, 3202)
        longitude                   3101 degrees_east, bound=(3100, 3102) degrees_east
        mesh_face_aux               0.0, bound=(0.0, 0.0)
    Cell methods:
        mean                        latitude
>>> 


@rcomer
Copy link
Member

rcomer commented Mar 29, 2022

That particular call to is_contiguous only exists to generate a warning about metadata

iris/lib/iris/coords.py

Lines 2215 to 2222 in 62047f7

elif not self.is_contiguous():
msg = (
"Collapsing a non-contiguous coordinate. "
"Metadata may not be fully descriptive for {!r}."
)
warnings.warn(msg.format(self.name()))
if self.has_bounds():

I have seen this warning many times, and have never found it useful. What happens if you just don't bother with the contiguity check?

@schlunma
Copy link
Contributor

In ESMValTool, we recently started improving our support for unstructured grids. This issue here hampers the application of our area_statistics operation (basically collapsing over latitude and longitude) to data on unstructured grids.

I tested commenting the elif statement linked by @rcomer above, which seems to fix the problem.

Would it make sense to change the lines above to something like

@@ -2228,12 +2228,22 @@ class Coord(_DimensionalMetadata):
                     "Metadata may not be fully descriptive for {!r}."
                 )
                 warnings.warn(msg.format(self.name()))
-            elif not self.is_contiguous():
-                msg = (
-                    "Collapsing a non-contiguous coordinate. "
-                    "Metadata may not be fully descriptive for {!r}."
-                )
-                warnings.warn(msg.format(self.name()))
+            else:
+                try:
+                    self._sanity_check_bounds()
+                except ValueError as exc:
+                    msg = (
+                        f"Cannot check if coordinate is contiguous: "
+                        f"{str(exc)}."
+                    )
+                    warnings.warn(msg.format(self.name()))
+                else:
+                    if not self.is_contiguous():
+                        msg = (
+                            "Collapsing a non-contiguous coordinate. "
+                            "Metadata may not be fully descriptive for {!r}."
+                        )
+                        warnings.warn(msg.format(self.name()))
 
             if self.has_bounds():
                 item = self.core_bounds()

?

I could open a PR if that makes sense.

@rcomer
Copy link
Member

rcomer commented Jun 23, 2022

There has been talk elsewhere (e.g. #4762 (comment)) of reducing the number of warnings Iris throws. Personally my vote would be to remove this if loop (and the two warnings it throws) altogether.

@schlunma
Copy link
Contributor

Sounds much easier! Can I open a pull request with that?

@rcomer
Copy link
Member

rcomer commented Jun 23, 2022

@schlunma I'd be delighted to merge such a PR, but I don't think I have the authority to decide that by myself. Hopefully someone else from @SciTools/peloton can weigh in. I guess it could be considered a breaking change, but I can't imagine anyone is relying on these warnings to tell them that their "metadata might not be fully descriptive".

@trexfeathers
Copy link
Contributor

trexfeathers commented Jul 20, 2022

@SciTools/peloton like @schunma's suggestion, thanks! Please submit something and we'll get it reviewed

@schlunma
Copy link
Contributor

See #4870.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants