PI-3483: Ensure collapsed/aggregated_by/rolling_window treat ancillary variables properly#3549
Conversation
stephenworsley
force-pushed
the
anc_collapse
branch
from
e111749 to
efcaaee
Compare
stephenworsley
force-pushed
the
anc_collapse
branch
from
efcaaee to
aba9844
Compare
stephenworsley
changed the title
trexfeathers
left a comment
trexfeathers
left a comment
There was a problem hiding this comment.
Hi @stephenworsley. Sorry but I think the tests need more refactoring to fully account for your changes.
|
Also, since this is a change in behaviour for cell measures, there will need to be a what's new entry |
trexfeathers
left a comment
trexfeathers
left a comment
There was a problem hiding this comment.
Tests look good! You might have missed it but I also added a comment that there should be a what's new for the changed cell measure behaviour. Thanks 👍
abooton
changed the title
abooton
changed the title
abooton
changed the title
| del cube.attributes[key] | ||
|
|
||
|
|
||
| def _remove_ancils_and_cms_mapping_dims(cube, dims): |
There was a problem hiding this comment.
After some consideration, I guess this is a reasonable place for it, as we already have other general-purpose private routines here.
I know it was my proposal, but can you think of a handier short name ?
(Anticipating complaint from @bjlittle as on this one !)
| npts = cube.shape[i_dim] | ||
| coord = DimCoord(np.arange(npts), long_name=name) | ||
| cube.add_dim_coord(coord, i_dim) | ||
| self.ancillary_variable = AncillaryVariable([0, 1], long_name="foo") |
There was a problem hiding this comment.
I think adding this to a testclass called specifically Test_collapsed__lazy is a bit cheeky !
The actual (pre-existing) testcases show that this class is specifically to test operation with lazy data.
So I think we should instead add a new testclass to be the home for these new tests.
As we don't have a "Test_collapsed", and we're not even aiming to do that here+now, I would just call it something like 'Test_collapsed__cellmeasure_ancils` + keep it minimal for this purpose.
By contrast, I think the changes to rolling-window and aggregated-by classes below are fine as they are.
|
Hi @stephenworsley, reviewed what is left to do, so here is my list of what's outstanding : |
pp-mo
left a comment
pp-mo
left a comment
There was a problem hiding this comment.
Hi @stephenworsley
One more small point came up.
Otherwise all done, great !
|
Nice work @stephenworsley, thanks for patience !! |
abooton
changed the title
abooton
changed the title
abooton
changed the title
abooton
changed the title
4 participants
Fixes a few issues with #3483. Rellies upon #3548 and #3546 being merged to work properly.