cube.aggregated_by and multidimensional auxcoords

[rcomer]

I had a go at fixing My First Issue #1530.

It's not quite there yet - see inline comments and failing new test.

[rcomer]

This doesn't quite work (and is the reason for the new test failing), as the longer resulting strings get truncated to match the shortest. E.g:

import numpy as np

a = np.arange(25).reshape(5, 5)
stra = a.astype(np.dtype('U'))

print(stra[:, 2:4])

print(np.apply_along_axis('|'.join, 1, stra.take([2, 3], 1)))

gives

[['2' '3']
 ['7' '8']
 ['12' '13']
 ['17' '18']
 ['22' '23']]
['2|3' '7|8' '12|' '17|' '22|']

Any suggestions how to fix this? I'm sure I could come up with a loop, but something slicker would be nice.

[rcomer]

I posted this on StackOverflow.
https://stackoverflow.com/questions/52677947

Based on the linked numpy GItHub issue I have a second solution. So I think the options are either a helper function like join_along_axis in my SO OP, or something along the lines of the answer I posted. Any preferences?

[rcomer]

Timings:
With join_along_axis timeit gives me:
500 loops, best of 3: 77 usec per loop

With the second solution:
500 loops, best of 3: 225 usec per loop

The second solution with the dtype calculation outside the loop:
500 loops, best of 3: 139 usec per loop

So I am very much leaning to using join_along_axis, as I think it also wins on readability.

[rcomer]

Somewhat belatedly realised that noone could see my inline comments until I hit “submit review” 😳

[DPeterK]

noone could see my inline comments until I hit “submit review"

Not to worry – we all do this all the time too! I'll see if I can take a look at this sometime this week...

[rcomer]

Multidimensional string coord test now fixed. For my rationale on the method, see the outdated diff comments for a conversation I had with myself! 😆

[corinnebosley]

@rcomer Awesome! I'm afraid @dkillick is not in this week to continue his review, but @kaedonkers will be taking a look instead this very afternoon.

[rcomer]

Thanks @corinnebosley - I am on leave for two weeks from Wednesday so, unless the review changes are very minor, they won't get done till November anyway.

[corinnebosley]

Thanks for all this effort @rcomer, I don't know how you find the time. I've added a few comments to your PR, mostly just aesthetic changes though. Functionally, this looks sound to me.

[rcomer]

Thanks @corinnebosley and @QuLogic for your feedback. I think I've covered everything except I've not changed the "simple" names in the tests (though happy to do so given a better suggestion).

[rcomer]

I've had a look through the test failures, and they don't seem to be related to aggregated_by. I'm quite far behind the master branch now - is a rebase likely to help?

Edit: have rebased as it can't do any harm!

[rcomer]

Rebase didn't help. 😞

[rcomer]

Have updated the whatsnew contributions directory name, as this is clearly not going into v2.2!

Looking more closely at the Travis failures, although they are popping up all over the place they all seem to come down datetime/calendar issues.

[corinnebosley]

@rcomer Yeah apologies about that, the failures are to do with some dependencies in the testing environment. I'll respin the tests once we've fixed the dependency issues.

[rcomer]

Hi @corinnebosley, I just noticed your latest PR so have rebased again. 🤞

[corinnebosley]

@rcomer Wow, you don't hang about do you? I'll ping you with some details later on.

[corinnebosley]

Nice work @rcomer! Thanks for the effort.