Skip to content

Partial Collapse of Multidimensional Auxcoords with Bounds #3302

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

Merged
lbdreyer merged 3 commits into from
Aug 19, 2019
Merged

Partial Collapse of Multidimensional Auxcoords with Bounds #3302

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f7165a
fix partial collapse of bounded multidim auxcoords
rcomer Apr 3, 2019
7deefda
add whatsnew
rcomer Apr 3, 2019
a1470f9
review changes
rcomer Jun 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...s/src/whatsnew/contributions_2.3.0/bugfix_2019-Apr-03_partial_collapse_bounded_coords.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
* :meth:`iris.cube.Cube.collapsed` now handles partial collapsing of
multidimensional coordinates that have bounds.
18 changes: 12 additions & 6 deletions lib/iris/coords.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1356,16 +1356,22 @@ def serialize(x):
'Metadata may not be fully descriptive for {!r}.'
warnings.warn(msg.format(self.name()))

# Determine the array library for stacking
al = da if self.has_bounds() \
and _lazy.is_lazy_data(self.core_bounds()) else np
if self.has_bounds():
item = self.core_bounds()
if dims_to_collapse is not None:
# Express main dims_to_collapse as non-negative integers
# and add the last (bounds specific) dimension.
dims_to_collapse = tuple(
dim % self.ndim for dim in dims_to_collapse) + (-1,)

@lbdreyer lbdreyer Jun 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

else:
item = self.core_points()

item = al.concatenate(self.core_bounds()) if self.has_bounds() \
else self.core_points()
# Determine the array library for stacking
al = da if _lazy.is_lazy_data(item) else np

# Calculate the bounds and points along the right dims
bounds = al.stack([item.min(axis=dims_to_collapse),
item.max(axis=dims_to_collapse)]).T
item.max(axis=dims_to_collapse)], axis=-1)
points = al.array(bounds.sum(axis=-1) * 0.5, dtype=self.dtype)

# Create the new collapsed coordinate.
Expand Down
42 changes: 41 additions & 1 deletion lib/iris/tests/unit/coords/test_Coord.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# (C) British Crown Copyright 2013 - 2018, Met Office
# (C) British Crown Copyright 2013 - 2019, Met Office
#
# This file is part of Iris.
#
Expand Down Expand Up @@ -303,6 +303,30 @@ def test_numeric_nd(self):
[4, 10],
[5, 11]]))

def test_numeric_nd_bounds(self):
self.setupTestArrays((3, 4))
coord = AuxCoord(self.pts_real, bounds=self.bds_real)

collapsed_coord = coord.collapsed()
self.assertArrayEqual(collapsed_coord.points, np.array([55]))
self.assertArrayEqual(collapsed_coord.bounds, np.array([[-2, 112]]))

# Test partially collapsing one dimension...
collapsed_coord = coord.collapsed(1)
self.assertArrayEqual(collapsed_coord.points, np.array([15, 55, 95]))
self.assertArrayEqual(collapsed_coord.bounds, np.array([[-2, 32],
[38, 72],
[78, 112]]))

# ... and the other
collapsed_coord = coord.collapsed(0)
self.assertArrayEqual(
collapsed_coord.points, np.array([40, 50, 60, 70]))
self.assertArrayEqual(collapsed_coord.bounds, np.array([[-2, 82],
[8, 92],
[18, 102],
[28, 112]]))

@lbdreyer lbdreyer Jun 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you split this into 3 tests? i.e. one test for each collapse operation.

My concern with having them all in the one test is that if the first one fails we won't know the result of the other two collapses until we get the first one working.
I realise test_numeric_nd() that already exists in this file does the same thing as you have done here, but I do think it's better practice to split the tests up

@lbdreyer lbdreyer Jun 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And perhaps add a test where the dimensions given are negative to test that handling is correct:
i.e.

collapsed_coord = coord.collapsed(-1)

@rcomer rcomer Jun 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will do, and thanks for explaining why - it makes sense. Should I do the same with the lazy equivalent below?

@lbdreyer lbdreyer Jun 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should I do the same with the lazy equivalent below?

Yes please!

@rcomer rcomer Jun 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. Also moved the dask import to the top of the module. This is consistent with other test modules that use dask.

Since I now have self.setupTestArrays((3, 4)) in 8 of the tests, I wondered about adding a setUp method to contain that. But there are a lot of tests on the class that don't need these arrays so not really sure what the better approach is.


def test_lazy_nd_bounds(self):
import dask.array as da

Expand All @@ -319,6 +343,22 @@ def test_lazy_nd_bounds(self):
self.assertArrayEqual(collapsed_coord.points, np.array([55]))
self.assertArrayEqual(collapsed_coord.bounds, da.array([[-2, 112]]))

# Test partially collapsing one dimension...
collapsed_coord = coord.collapsed(1)
self.assertArrayEqual(collapsed_coord.points, np.array([15, 55, 95]))
self.assertArrayEqual(collapsed_coord.bounds, np.array([[-2, 32],
[38, 72],
[78, 112]]))

# ... and the other
collapsed_coord = coord.collapsed(0)
self.assertArrayEqual(
collapsed_coord.points, np.array([40, 50, 60, 70]))
self.assertArrayEqual(collapsed_coord.bounds, np.array([[-2, 82],
[8, 92],
[18, 102],
[28, 112]]))

def test_lazy_nd_points_and_bounds(self):
import dask.array as da

Expand Down