Skip to content

Cube.aggregated_by doesn't handle multi-dimensional AuxCoords #1530

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

Closed
rcomer opened this issue Jan 19, 2015 · 5 comments
Closed

Cube.aggregated_by doesn't handle multi-dimensional AuxCoords #1530

rcomer opened this issue Jan 19, 2015 · 5 comments

Comments

@rcomer
Copy link
Member

rcomer commented Jan 19, 2015

This follows from a discussion on the Google Group. If an AuxCoord spans more than one dimension, and Cube.aggregated_by is applied over one of those dimensions, the AuxCoord points are simply truncated.

Here's a simple example:

import iris
import numpy as np

dim_coord1 = iris.coords.DimCoord(range(3), long_name='first_dim')
dim_coord2 = iris.coords.DimCoord(range(4), long_name='second_dim')

aux_coord2 = iris.coords.AuxCoord([0,0,1,1], long_name='aggregation_coord')
aux_coord_2d = iris.coords.AuxCoord(np.arange(12).reshape(3,4),
                                                     long_name='spanning')

cube = iris.cube.Cube(np.ones((3,4)),
                      dim_coords_and_dims=[(dim_coord1,0), (dim_coord2,1)],
                      aux_coords_and_dims=[(aux_coord2, 1), (aux_coord_2d, (0,1))])

newcube = cube.aggregated_by('aggregation_coord', iris.analysis.SUM)

print newcube.coord('spanning')

This gives:

AuxCoord(array([[0, 1],
       [4, 5],
       [8, 9]]), standard_name=None, units=Unit('1'), long_name='spanning')

which is inconsistent with what we get when applying the aggregation to slices of the cube:

for i in range(3):
    print cube[i].aggregated_by('aggregation_coord', iris.analysis.SUM).coord('spanning')

AuxCoord(array([ 0.5,  2.5]), bounds=array([[0, 1],
       [2, 3]]), standard_name=None, units=Unit('1'), long_name='spanning')
AuxCoord(array([ 4.5,  6.5]), bounds=array([[4, 5],
       [6, 7]]), standard_name=None, units=Unit('1'), long_name='spanning')
AuxCoord(array([  8.5,  10.5]), bounds=array([[ 8,  9],
       [10, 11]]), standard_name=None, units=Unit('1'), long_name='spanning')
@pelson
Copy link
Member

pelson commented Feb 2, 2015

Thanks for this @rcomer - particularly for providing a simple (data less!) cube to reproduce. This will make writing a test for any new behaviour much, much easier! 👍

@rcomer
Copy link
Member Author

rcomer commented Sep 14, 2018

I thought I'd have a crack at fixing this, and have a new branch over here. The above example works sensibly with my changes, but now I need to think about writing proper tests.

Where should the tests go? I have found tests for aggregated_by in 3 different places:

Also feel free to tell me if I shouldn't bother...!

@DPeterK
Copy link
Member

DPeterK commented Sep 17, 2018

I thought I'd have a crack at fixing this

Thanks! An excellent question about the tests, too. I think the answer is hinted at here in the Iris dev guide – the first tests you've found are the integration tests (checking that the functionality being tested works within the broader setting of Iris), the second set are the current unit tests (checking that the individual code units on their own do what's expected) and the third are the legacy unit tests left over from before we moved to having specific unit and integration test directories.

So – focus on new unit tests as required in tests/unit/... and ensuring existing tests there and in tests/integration work. If the legacy tests don't work then consider deleting them! But if possible it's good to ensure that what the legacy tests are checking is still checked elsewhere.

Again, thanks for taking this on! Hope the testing isn't too arduous...

@rcomer
Copy link
Member Author

rcomer commented Sep 17, 2018

Thanks for the explanation @dkillick. There's actually very little in the current unit tests (which is where I looked first and was somewhat surprised!) The legacy tests are a lot more comprehensive so I'm thinking they are still necessary - and they pass with my change in any case.

@rcomer
Copy link
Member Author

rcomer commented Nov 7, 2018

Fixed at #3174.

@rcomer rcomer closed this as completed Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants