Skip to content
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

PI-3473: Add Ancillary Variable support to merge #3569

Merged
merged 3 commits into from
Dec 17, 2019

Conversation

stephenworsley
Copy link
Contributor

@stephenworsley stephenworsley commented Nov 29, 2019

Addresses half of #3552. Now merge treats Ancillary Variables the same as Cell Measures.

@stephenworsley stephenworsley changed the title Add Ancillary Variable support to merge PI-3473: Add Ancillary Variable support to merge Dec 6, 2019
@pp-mo pp-mo self-assigned this Dec 10, 2019
@pp-mo pp-mo self-requested a review December 10, 2019 11:06
@pp-mo
Copy link
Member

pp-mo commented Dec 12, 2019

Now merge treats Ancillary Variables the same as Cell Measures.

Unfortunately I think this approach is probably wrong, as discussed offline with @bjlittle ...

NOTE related prior discussions #3603 #3084 #3234

We think that not ever merging (stacking) cell-measures does make ok sense, as these generally describe the "data spatial sampling grid" in dims X, Y and possibly Z.
( So, we expect that "grid" to be probably complete within each source file, and not extend into "extra" dimensions seen only by merging multiple netcdf files ).

However in contrast, Auxiliary Variables can easily map over any and all dimensions of the data, particularly when they are used as status flags : they could have a value at every single datapoint.
So Ancillaries should be stacked up by merging in a manner analogous to the data itself.

( Actually, I had thought that merge would do this with AuxCoords too, but it seems we have chosen not to.
I only just found this out 😉 )


On the other hand ...

I see this was not in the original Acceptance Criteria on #3552, as opposed to the equivalent for concatenate :

  • cubes with similar ancillary variables which span the axis of concatenation, will concatenate to form a new ancillary variable which spans the concatenated cube
  • cubes with differing ancillary variables (i.e. same names but some difference) will not merge;

So, having seen that we can't currently do this with AuxCoords either, I wonder..
can we leave this as extended functionality for later delivery ?

@stephenworsley has shown that we can work-around these cases via concatenate :

  • .. using first iris.util.new_axis to create the new ("merge") dimension ..
  • .. then a fairly short utility to "promote" Ancillaries (?and AuxCoords) to mapping the 'new axis',
    • ( roughly : replace with reshaped version, with extra length-1 dimension and new-dim added into dims-map )
  • .. then resulting cubes will concatenate to the required result

See proposed extension to new_axis : #3600

@stephenworsley
Copy link
Contributor Author

can we leave this as extended functionality for later delivery ?

@pp-mo Does this mean that iris 3.0 should discard/ignore ancillary variables on merge? If this is the case, perhaps a warning should be given when this happens.
Or are you suggesting we keep the current functionality of the pull request with a view that this can be changed/extended in the future?

@pp-mo
Copy link
Member

pp-mo commented Dec 13, 2019

@stephenworsley Does this mean that iris 3.0 should discard/ignore ancillary variables on merge? ... Or are you suggesting we keep the current functionality of the pull request

The latter I think: That is, not discard but only carry over matching versions. A statement of that would be useful, somewhere but frankly I'm not sure where. Suggest if you like..

I still need to check this out further, including test coverage, but I will need a bit longer. Sorry to be so slow today.

@pp-mo
Copy link
Member

pp-mo commented Dec 17, 2019

Ok, after long thinks about the concatenate #3566, I've convinced myself that this one really is much simpler. Thanks @stephenworsley !

@pp-mo pp-mo merged commit 4fc08bb into SciTools:master Dec 17, 2019
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

Successfully merging this pull request may close these issues.

2 participants