Adding a utility for flattening Aux Coords #3030

duncanwp · 2018-05-24T22:09:23Z

e.g. for later use in extract calls, and other use cases (cf #2766).

It still needs a couple of tests and some docs but I'd like some feedback on the general usefulness and approach first.

See discussion here: https://groups.google.com/forum/#!topic/scitools-iris/Kni1_QO7H1M

Ping @rcomer who also has an implementation of this!

rcomer · 2018-05-25T09:28:38Z

lib/iris/util.py

+        if not isinstance(c, DimCoord):
+            # The new (flat) AuxCoords are always the last dimension
+            new_aux_coords.append((c.copy(c.points.flat,
+                                          c.bounds.reshape(-1, 2)),


I got an error here because bounds is None.

I'm also not sure if this is robust to the cases where AuxCoord dimensions are not necessarily in the order you expect (#2606).

I'll fix that

rcomer · 2018-05-25T09:31:52Z

lib/iris/util.py

+        msg = msg.format(type(name_or_coord))
+        raise TypeError(msg)
+
+    coord_dims = cube.coord_dims(coord)


Is it worth adding something like

if len(coord_dims) == 1: return cube

I've just twigged that the auxcoord of interest gets moved to the last dimension when flattened, so perhaps a cube.transpose would be more consistent.

It may also be worth having some kind of handling for coords which span more than 2 dims (which I am assured can exist). However, considering how rare they must be, I would be happy with a not-implemented error in that case, or maybe just a pointer to flatten_cube.

rcomer · 2018-05-25T09:47:54Z

lib/iris/util.py

+    new_data = cube.data.reshape(new_shape)
+    new_aux_coords = other_aux_coords
+    for c in coords_to_flatten:
+        # Only flatten aux coords (we will drop DimCoords)


My preference would be for the DimCoords to be converted to AuxCoords, with their points and bounds repeated so they fit along the flattened axis.

I'd like the 1D AuxCoords on these dimensions to be treated similarly.

1d AuxCoords should survive untouched

rcomer · 2018-05-25T09:49:39Z

lib/iris/util.py

+    new_shape = np.hstack([shape_array[other_dims],
+                           np.product(shape_array[list(coord_dims)])])
+
+    new_data = cube.data.reshape(new_shape)


Could this be done on the dask array if the cube has lazy data?

Yes, data.reshape should work on both numpy and dask arrays

rcomer · 2018-05-25T10:27:38Z

I'd definitely be pleased to see something like this included in Iris, and your implementation looks a lot cleaner than mine!

In case it helps motivate a merge, I'll explain my own use case: I'm analysing GloSea hindcast ensembles. Sometimes the cube looks like this (I added the combined_realization coord as a function of the realization and forecast_reference_time coords):

air_temperature / (K)               (realization: 4; -- : 108; latitude: 324; longitude: 432)
     Dimension coordinates:
          realization                           x       -              -               -
          latitude                              -       -              x               -
          longitude                             -       -              -               x
     Auxiliary coordinates:
          combined_realization                  x       x              -               -
          forecast_period                       -       x              -               -
          forecast_reference_time               -       x              -               -
          time                                  -       x              -               -
     Scalar coordinates:
          height: 1.5 m
     Attributes:
          STASH: m01s03i236
          source: Data from Met Office Unified Model
          um_version: 10.4
     Cell methods:
          mean: time (1 hour)

But if a member falls over it gets replaced with a higher numbered one, so the cube looks like this:

air_temperature / (K)               (-- : 432; latitude: 324; longitude: 432)
     Dimension coordinates:
          latitude                      -              x               -
          longitude                     -              -               x
     Auxiliary coordinates:
          forecast_period               x              -               -
          forecast_reference_time       x              -               -
          realization                   x              -               -
          time                          x              -               -
     Scalar coordinates:
          height: 1.5 m
     Attributes:
          STASH: m01s03i236
          source: Data from Met Office Unified Model
          um_version: 10.4
     Cell methods:
          mean: time (1 hour)

If I flatten out the combined_realization coord, then my cubes will always have the same shape. This makes writing subsequent analysis code simpler!

stickler-ci · 2018-05-31T22:12:44Z

lib/iris/util.py

+                  longitude                   x
+    """
+    import numpy as np
+    from iris.coords import AuxCoord


F401 'iris.coords.AuxCoord' imported but unused

duncanwp · 2018-05-31T22:15:00Z

Thanks for the useful feedback!

The latest commit should include all of your suggestions, and I've added some tests. It would be great to include your use-case as a test though - do you have some code I could use to mock up a cube with those coordinate?

This still needs a whats-new if it's to be merged

duncanwp · 2018-06-01T06:37:08Z

Note to self: it might be nice to factor out a method for flattening a cube along specific dimensions (which is what the meat of this function really does). I should also check that creating a new cube is necessary and desirable - and if so that the new coord points aren’t just views on the old ones.

rcomer · 2018-06-01T10:18:22Z

lib/iris/util.py

+                        if c not in coords_to_flatten
+                        and c not in coords_to_expand]
+
+    new_data = cube.data.reshape(new_shape)


Think this needs cube.core_data() if it's to work on Dask arrays.

I also think the array might need transposing if the auxcoord didn't start off on the trailing dimensions. Sorry, I missed that before.

Yes, I need to get used to using core_data() now!

OK, I think it will now work with Aux Coords across any combination of dimensions, I've added a couple of tests too.

rcomer · 2018-06-01T10:24:08Z

lib/iris/util.py

+        new_points = broadcast_to_shape(c.points, coord.shape, (0,))
+        new_bounds = broadcast_to_shape(c.bounds, coord.bounds.shape, (0,)) \
+            if c.bounds is not None else None
+        coords_to_flatten.append(c.copy(new_points, new_bounds))


Test is failing here because its trying to create a 2d DimCoord. You can get around it with

coords_to_flatten.append( iris.coords.AuxCoord.from_coord(c).copy(new_points, new_bounds))

though there may be a better way.

Then the test still fails when the cube is constructed because it claims the new 'blah' is only length 3. I do not understand this at all.

Yes, good catch thanks - I've fixed that now.

rcomer · 2018-06-01T10:26:08Z

lib/iris/util.py

+
+    # Expand the coords which need expanding first, then add to flatten list
+    for c in coords_to_expand:
+        new_points = broadcast_to_shape(c.points, coord.shape, (0,))


The (0,) here will need generalising I think.

Actually, although it looks a bit wierd the zero here is fine because c is must be one dimensional. I've added a comment to clarify

c is one dimensional, but coord is multidimensional, and the tuple indicates the dimension of coord that c is getting mapped to. So, for your docstring example, you would have
broadcast_to_shape(c.points, (2, 2), (0,)) for the x coord, but
broadcast_to_shape(c.points, (2, 2), (1,)) for the y coord.

Ah, yes - sorry! 😳

Is there a nice way of getting the dimension of an Aux Coord for a given DimCoord?

No specific method that I know of. I'd be tempted to combine the loop with the one at line 1614.

for dim_map, d in enumerate(coord_dims): for c in cube.coords(dimensions=d): new_points = broadcast_to_shape(c.points, coord.shape, (dim_map,)) ...etc...

rcomer · 2018-06-01T10:30:30Z

do you have some code I could use to mock up a cube with those coordinate?

I don't at the moment but will have a go when I have a minute.

stickler-ci · 2018-06-03T21:36:54Z

lib/iris/tests/unit/util/test_flatten_multidim_coord.py

+    def test_aux_coords_leading(self):
+        cube_a = stock.simple_3d_w_multidim_coords()
+        # Move the aux coord dims to the front of the cube
+        cube_a.transpose((1,2,0))


E231 missing whitespace after ','

stickler-ci · 2018-06-03T21:36:54Z

lib/iris/tests/unit/util/test_flatten_multidim_coord.py

+
+    def test_split_aux_coord_dims(self):
+        cube_a = stock.simple_3d_w_multidim_coords()
+        cube_a.transpose((1,0,2))


E231 missing whitespace after ','

duncanwp · 2018-06-03T21:41:31Z

Does anyone have a preference for if this should work on the Cube in-place? It would have to reach into the Cube internals to update the coordinates which then makes it feel more like a method on the Cube itself, but maybe it's OK?

stickler-ci · 2018-06-05T10:01:34Z

lib/iris/util.py

+    # These (1-D) coordinates are expanded before being flattened
+    coords_to_ignore = []
+    for aux_dim, coord_dim in enumerate(coord_dims):
+        # Expand the coords which need expanding first, then add to flatten list


E501 line too long (80 > 79 characters)

stickler-ci · 2018-06-05T10:01:34Z

lib/iris/util.py

+            new_bounds = None if c.bounds is None else \
+                broadcast_to_shape(c.bounds, coord.bounds.shape, (aux_dim,))
+            coords_to_flatten.append(
+                iris.coords.AuxCoord.from_coord(c).copy(new_points, new_bounds))


E501 line too long (80 > 79 characters)

stickler-ci · 2018-06-05T10:51:38Z

lib/iris/util.py

+                  latitude                    x
+                  longitude                   x
+    """
+    from iris.coords import AuxCoord


F401 'iris.coords.AuxCoord' imported but unused

duncanwp · 2018-06-05T10:55:59Z

OK, I don't think I'll be able to do this inlace since the DataManager explicitly doesn't allow these kind of reshapes (see _data_manager.py:L260). Is there a way around this?

I had come to the conclusion that since we're just reshaping it would be nice not to have to copy the data...

duncanwp · 2018-06-12T11:50:27Z

@pelson any ideas?

pp-mo · 2018-06-22T11:20:38Z

An inplace operation would definitely need to be a Cube method, as at present the cube API is designed to guarantee that any cube derived from another one is totally independent, i.e. no data sharing.
However, this is something we are intending to relax in future, though we are currently a bit blocked on that -- see #2681.

The guaranteed copying behaviour seemed like a great idea at outset of Iris, because of the total certainty given, and at least partly because many of our internal users were used to IDL which did make copies in all cases.
Going forward we can instead probably adopt the numpy attitude, which I'd summarise as
"for efficiency, results can sometimes share data with the input; you just need to be careful".

rcomer · 2018-10-05T13:05:21Z

I've finally got around to testing the latest revision with my data and can report that it's working nicely now:

In [14]: print(cube)
air_temperature / (K)               (realization: 4; -- : 108; latitude: 324; longitude: 432)
     Dimension coordinates:
          realization                           x       -              -               -
          latitude                              -       -              x               -
          longitude                             -       -              -               x
     Auxiliary coordinates:
          combined_realization                  x       x              -               -
          forecast_period                       -       x              -               -
          forecast_reference_time               -       x              -               -
          time                                  -       x              -               -
     Scalar coordinates:
          height: 1.5 m
     Attributes:
          STASH: m01s03i236
          source: Data from Met Office Unified Model
          um_version: 10.4
     Cell methods:
          mean: time (1 hour)

In [15]: print(iris.util.flatten_multidim_coord(cube, 'combined_realization'))
air_temperature / (K)               (-- : 432; latitude: 324; longitude: 432)
     Dimension coordinates:
          latitude                      -              x               -
          longitude                     -              -               x
     Auxiliary coordinates:
          combined_realization          x              -               -
          forecast_period               x              -               -
          forecast_reference_time       x              -               -
          realization                   x              -               -
          time                          x              -               -
     Scalar coordinates:
          height: 1.5 m
     Attributes:
          STASH: m01s03i236
          source: Data from Met Office Unified Model
          um_version: 10.4
     Cell methods:
          mean: time (1 hour)

I did start writing some code to mock up a cube with similar metadata, but it got messy very quickly 😟.

corinnebosley

Thanks for all this work @duncanwp, your time and effort is appreciated.

Functionally, this looks great, but I have made some comments as I think there are some minor changes to make before we get this merged.

Most of the changes are just typos and name changes, but I do think the tests need tidying up a bit; they're a bit mixed up at the moment and all seem to be testing several things at once. I have commented on the tests individually, but you will also need to make a new file called iris.tests.unit.util.test_flatten_cube and move the appropriate tests across.

lib/iris/util.py

corinnebosley · 2018-10-24T10:32:32Z

lib/iris/util.py

+        msg = msg.format(type(name_or_coord))
+        raise TypeError(msg)
+
+    coord_dims = cube.coord_dims(coord)


It may also be worth having some kind of handling for coords which span more than 2 dims (which I am assured can exist). However, considering how rare they must be, I would be happy with a not-implemented error in that case, or maybe just a pointer to flatten_cube.

lib/iris/tests/unit/util/test_flatten_multidim_coord.py

corinnebosley · 2018-10-24T14:14:16Z

lib/iris/tests/unit/util/test_flatten_multidim_coord.py

+        self.assertEqual(cube_b.dim_coords, tuple())
+        self.assertEqual(cube_b.shape, (12, ))
+
+    def test_oned_dim_coord_flattened(self):


The odd comment with an explanation of the point of the test wouldn't go amiss.

corinnebosley · 2018-10-24T14:28:24Z

lib/iris/tests/unit/util/test_flatten_multidim_coord.py

+        self.assertEqual(cube_b.coord('blah', dim_coords=False).shape, (12, ))
+        self.assertEqual(cube_b.shape, (2, 12))
+
+    def test_multiple_oned_dim_coords_flattened(self):


I don't think this test name describes the test very well. The word 'oned' seems inappropriate as one of your additional coords is actually twoed (or they are both just inserted).

I'm also not sure what this test adds compared to that above, so I would recommend removing it (considering the heaving test suite that Iris already accommodates, any new tests added should be lean and purposeful).

I've tried to rationalise these a bit now and added a comment to describe them

corinnebosley · 2018-10-24T14:36:44Z

lib/iris/tests/unit/util/test_flatten_multidim_coord.py

+        cube_b = flatten_cube(cube_a, (0, 2))
+        self.assertEqual(cube_b.dim_coords, (cube_a.coord('wibble'), ))
+        self.assertEqual(cube_b.shape, (12, 2))
+


Again, I'm not sure if this test adds any value that hasn't already been added by test_aux_coords_leading, so I would suggest removing it.

This is useful because there is some special logic for dealing with these types of coordinates

…ract calls) Adding some tests and fixing some edge cases Removing unused import Add comment and rename var for clarity Add comment and rename var for clarity Minor fix Retain lazy data Support aux coords on leading dimensions, and split across non-neighbouring dimensions. The routine chooses the first dimension of the AuxCoord as the new flat one. Fix whitespace Fix issue with multiple dim coords being flattened PEP8 conformance Long lines Factor out a function for flattening arbitrary dimensions Fix case for coordinates with bounds Flatten all dims by default Remove unused imports Remove unused imports

…ions. Other minor whitespace and text changes

duncanwp · 2019-01-20T22:49:35Z

Thanks for the detailed review @corinnebosley. I've finally managed to get around to making your proposed changes. Hopefully it's looking a bit tidier now.

* BLD: Pinning cftime to 1.0.1 (SciTools#3247) * Fix for numpy 1v16. (SciTools#3257)

…ools#3267)" (SciTools#3274) This reverts commit a76afdc.

duncanwp · 2019-06-13T17:12:48Z

@bjlittle I consider this done now, rather than work in progress. Do you think there's something missing?

bjlittle · 2019-06-14T17:08:16Z

Hey @duncanwp, awesome! I'll take a peek and start reviewing...

Before I do, would you consider it a feature enhancement that's not a breaking change? We're teeing up iris minor release 2.3.0 for the end of August, and iris major release 3.0.0 for November...

duncanwp · 2019-06-14T17:10:22Z

It's just an extra feature and not breaking so if you can roll it into 2.3.0 that would be great!

pp-mo · 2019-08-23T16:05:56Z

Hi @duncanwp
We're working on iris 2.3 and I'm just considering this PR, but the history has got into a nasty mess for some reason...

The whole of a 2.2.x mergeback to master (#3256) is appearing on your commit chain, and the main set of changes with your name appear twice in 2 different versions !
I think the penultimate commit "Merge remote-tracking branch 'origin/flatten_aux_coord' into flatten_aux_coord" is probably the main culprit, it sounds like a mistake to merge from your fork back into your local ?

So I rebuilt by just cherry-picking the final commits, which then looks like #3379.
Does that contain everything it should have, in your latest incarnation ??

Apart from that I think may be a few remaining things to pick up on, but I guess we need you to fix the PR, i.e. reconfirm what is proposed first.
Can you find some time to fix this ?

pp-mo · 2019-08-27T10:48:20Z

Note my (failed!) attempt to expedite this : #3379

There are outstanding relevant comments there :

duncanwp · 2019-08-30T09:23:47Z

That's frustrating, I thought it was ready :-(

I won't have a chance to look at this for a while but I'll pick it up when I have a chance. Thanks for trying to expedite it.

pp-mo · 2019-08-30T12:53:12Z

Thanks for your attention @duncanwp .
We'll wait !

bjlittle · 2020-10-20T08:38:43Z

@duncanwp Sadly this PR has been stalled for almost 2 and a half years now.

Rather than let it linger still longer, I'm going to close this PR, which is not something that I really want to do. Personally, I'd rather see it merged and its benefit utilised by the community.

However, if you have the time and motivation, then please re-open this PR and we'll take it from there. Hopefully we can get it across the line. Thanks 👍

rcomer reviewed May 25, 2018

View reviewed changes

duncanwp force-pushed the flatten_aux_coord branch from 2fc7161 to f03b1f2 Compare May 31, 2018 22:12

stickler-ci reviewed May 31, 2018

View reviewed changes

rcomer reviewed Jun 1, 2018

View reviewed changes

stickler-ci reviewed Jun 3, 2018

View reviewed changes

stickler-ci reviewed Jun 5, 2018

View reviewed changes

lib/iris/util.py Outdated

latitude x

longitude x

"""

from iris.coords import AuxCoord

Copy link

stickler-ci Jun 5, 2018

Choose a reason for hiding this comment

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

F401 'iris.coords.AuxCoord' imported but unused

corinnebosley reviewed Oct 24, 2018

View reviewed changes

duncanwp and others added 4 commits January 20, 2019 21:31

Adding a NotImplemented error for coordinates with more than 2 dimens…

4b19906

…ions. Other minor whitespace and text changes

Minor refactoring to tidy up tests

18a563d

Split out flatten_cube tests

38af4b5

duncanwp force-pushed the flatten_aux_coord branch from b1c9870 to 38af4b5 Compare January 20, 2019 22:46

pp-mo and others added 4 commits February 1, 2019 13:10

Mergeback 2.2.x (SciTools#3256)

6a4f9dc

* BLD: Pinning cftime to 1.0.1 (SciTools#3247) * Fix for numpy 1v16. (SciTools#3257)

Test with newest cf-units: no longer called cf_units. (SciTools#3265)

0f519e1

Avoid numpy FutureWarning about indexing with sequence. (SciTools#3267)

a76afdc

Revert "Avoid numpy FutureWarning about indexing with sequence. (SciT…

a7aa2c7

…ools#3267)" (SciTools#3274) This reverts commit a76afdc.

bjlittle removed the Status: Work in Progress label Jun 14, 2019

bjlittle self-assigned this Jun 14, 2019

bjlittle added this to the v2.3.0 milestone Jun 14, 2019

bjlittle added Feature: NetCDF + CF-conventions Release: Minor and removed Feature: NetCDF + CF-conventions labels Jun 14, 2019

pp-mo mentioned this pull request Aug 23, 2019

Rebuilt flatten auxcoords #3379

Closed

bjlittle added the Sprint: Refine me label Nov 13, 2019

bjlittle modified the milestones: v2.3.0, v3.1.0 Nov 13, 2019

bjlittle added the Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton label Oct 1, 2020

bjlittle added the Status: Stalled label Oct 20, 2020

bjlittle closed this Oct 20, 2020

Adding a utility for flattening Aux Coords #3030

Adding a utility for flattening Aux Coords #3030

Conversation

duncanwp commented May 24, 2018

rcomer May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer May 25, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented May 25, 2018

Choose a reason for hiding this comment

duncanwp commented May 31, 2018

duncanwp commented Jun 1, 2018

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

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

rcomer commented Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncanwp commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncanwp commented Jun 5, 2018

duncanwp commented Jun 12, 2018

pp-mo commented Jun 22, 2018 • edited Loading

rcomer commented Oct 5, 2018

corinnebosley left a 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

Choose a reason for hiding this comment

duncanwp commented Jan 20, 2019

duncanwp commented Jun 13, 2019

bjlittle commented Jun 14, 2019

duncanwp commented Jun 14, 2019

pp-mo commented Aug 23, 2019

pp-mo commented Aug 27, 2019

duncanwp commented Aug 30, 2019 • edited Loading

pp-mo commented Aug 30, 2019

bjlittle commented Oct 20, 2020

rcomer May 25, 2018 •

edited

Loading

rcomer May 25, 2018 •

edited

Loading

rcomer commented Jun 1, 2018 •

edited

Loading

pp-mo commented Jun 22, 2018 •

edited

Loading

duncanwp commented Aug 30, 2019 •

edited

Loading