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

Ancillary Variables - validate 'all other' cube operations #3554

Closed
7 tasks done
trexfeathers opened this issue Nov 20, 2019 · 7 comments
Closed
7 tasks done

Ancillary Variables - validate 'all other' cube operations #3554

trexfeathers opened this issue Nov 20, 2019 · 7 comments
Milestone

Comments

@trexfeathers
Copy link
Contributor

trexfeathers commented Nov 20, 2019

Status update 2019-12-09
It appears that most of the points raised + discussed below may now have been ruled out.

Current 'TODO' list (including things already-complete) :

Notes:

  • This ticket applies to "all" cube operations that produce a new cube, where an input cube with ancillaries might give a result with "wrong" ancillaries. For instance, where "old" ancillaries could now have the wrong dimensions.
  • It also covers some documentation, somehow, of "what" operations will do with ancils.
    • However, we may want to defer those additions until we are ready to "release" ancils, as described below. See separate ticket #3570
    • It might also be desirable to add matching notes for cell-measures handling, which are currently absent pretty much anywhere.

Use of launch_ancils feature-branch
Pending a final decision to "release" ancils in Iris 3.0, we need to defer documentation changes to the feature branch. See key note here
That applies also to the documentation of cube operations, as mentioned below : see separate ticket #3570 to cover this


Original notes :
original name : "Ancillary Variables - cube.copy invalid retention of AV's #3554"

The following include uses of cube.copy, which in #3546 will newly retain Cell Measures and Ancillary Variables. In these cases this could be inappropriate since the CM's/AV's could refer to dimensions that no longer exist.

As noted in #3483 there is a wider discussion about regridding/interpolation to be had since depending on the dimensions altered it may be appropriate to retain CM's/AV's. Until that is tackled in detail it is important the below operations do not behave in a misleading way and therefore a blanket discarding of CM's/AV's would be safest.

  • analysis._regrid.CurvilinearRegridder
  • analysis.trajectory.UnstructuredNearestNeigbourRegridder, also __call__ method
  • experimental.stratify.relevel

 

If the above are returning cubes with CM's/AV's:
-[ ] alter the functions/methods to prevent retention of CM's/AV's
-[ ] write tests to demonstrate this
-[ ] document this behavior
-[ ] ensure that the cases covered here are accounted for in any further discussions/work to conditionally retain/discard CM's/AV's depending on the dimensions involved

Note: the above tickboxes have been deprecated and I have edited them so that they don't contribute to the status bar.

@stephenworsley
Copy link
Contributor

From what I can tell looking at the code, (I haven't run tests yet) analysis._regrid.CurvilinearRegridder should discard any ancillary variables unless code is added to this function to account for them:

def _regrid_weighted_curvilinear_to_rectilinear__perform(
The last time the copy is used is in this code which makes a fresh cube using only the shape of the copy (which is here called grid_cube):
cube = iris.cube.Cube(
weighted_mean.reshape(grid_cube.shape),
dim_coords_and_dims=dim_coords_and_dims,
)

@stephenworsley
Copy link
Contributor

experimental.stratify.relevel also looks like it shouldn't copy over ancillary variables since the only time a cube is copied, only its metadata is extracted.

result = Cube(new_data, **cube.copy().metadata._asdict())

@stephenworsley
Copy link
Contributor

When analysis.trajectory.UnstructuredNearestNeigbourRegridder is called, the copied source cube is first passed to interpolate:

result_trajectory_cube = interpolate(
src_cube, self.trajectory, method="nearest"
)
The resulting cube is then recreated from its data, metadata and coordinates:
data_2d_x_and_y = result_trajectory_cube.data.reshape(target_shape)
# Make a new result cube with the reshaped data.
result_cube = iris.cube.Cube(data_2d_x_and_y)
result_cube.metadata = src_cube.metadata
# Copy all the coords from the trajectory result.
i_trajectory_dim = result_trajectory_cube.ndim - 1
for coord in result_trajectory_cube.dim_coords:
dims = result_trajectory_cube.coord_dims(coord)
if i_trajectory_dim not in dims:
result_cube.add_dim_coord(coord.copy(), dims)
for coord in result_trajectory_cube.aux_coords:
dims = result_trajectory_cube.coord_dims(coord)
if i_trajectory_dim not in dims:
result_cube.add_aux_coord(coord.copy(), dims)

This result_cube is then returned. It shouldn't contain any ancillary variables. The only problems left would have to be inside the interpolate call.

@pp-mo
Copy link
Member

pp-mo commented Nov 28, 2019

@stephenworsley "experimental.stratify.relevel also looks like it shouldn't copy over ancillary variables since the only time a cube is copied, only its metadata is extracted. "
result = Cube(new_data, **cube.copy().metadata._asdict())

That frankly looks like a bit of bug to me + a stupid thing to do : copy the whole cube to get an indepenedent (scalar) metadata copy, ? really ?
The only reason I can see for copying the cube before taking metadata it is that the _as_dict() does not copy objects in the dict values, which I guess would only really affect cell_methods anyway. It would make sense to fix that with a deepcopy of the dict, I would think.
A better coding solution should really be found. Especially as _as_dict isn't even a public method.

@pp-mo pp-mo changed the title Ancillary Variables - cube.copy invalid retention of AV's Ancillary Variables - validate 'all other' cube operations Dec 9, 2019
@stephenworsley
Copy link
Contributor

One more thing to be aware of, iris.util.new_axis is currently discarding cell measures and ancillary variables. This is probably worth addressing if we're expecting people to use it and concatenate to bypass the restrictions around merge.

@stephenworsley
Copy link
Contributor

One more thing that's probably worth noting. The iris.cube.Cube methods remove_ancillary_variable, ancillary_variable_dims and cell_measure_dims do not currently accept a name (string) as a valid argument. See #3295 for a simmilar case where this was already adressed for remove_cell_measure.

@pp-mo
Copy link
Member

pp-mo commented Dec 20, 2019

Ticked all the boxes, at last !

@pp-mo pp-mo closed this as completed Dec 20, 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

No branches or pull requests

3 participants