Skip to content

Conversation

stephenworsley
Copy link
Contributor

Fixes another issue in #3483.

@stephenworsley stephenworsley changed the title Fix copy to include Ancillary variables and Cell measures Fix copy to include Ancillary variables and Cell measures (DO NOT MERGE) Nov 18, 2019
@stephenworsley
Copy link
Contributor Author

This should be merged after rebasing with #3536 so that equality works for testing.

@stephenworsley
Copy link
Contributor Author

This has now been rebased with #3536.

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change, and please confirm whether this is still "DO NOT MERGE" since the rebasing has been done? Thanks @stephenworsley 😊

@stephenworsley stephenworsley changed the title Fix copy to include Ancillary variables and Cell measures (DO NOT MERGE) Fix copy to include Ancillary variables and Cell measures Nov 19, 2019
@trexfeathers
Copy link
Contributor

While AV's are a new feature, CM's are not. So I think there should be a what's new bugfix entry for the fact that CM's are now copied

@stephenworsley
Copy link
Contributor Author

I have patched in a rough proof of concept fix for equality. This ought to be improvable once #3551 is merged.

@trexfeathers
Copy link
Contributor

Notes on the effects on all uses of cube.copy. See below for a full list of uses. For those where a problem may be caused I have created #3554, as the problems are relatively minor and can be handled separately, so that this more important fix can proceed now.

  • analysis.PercentileAggregator.post_process
  • analysis.maths._math_op_common
    • allows AV's and CM's to be retained after performing arithmetic on a cube - the first cube if multiple cubes are involved
    • no foreseen issues
  • analysis.calculus.cube_delta
    • actually needs this copy fix now that __getitem__ has been fixed in __getitem__ gets ancillary variables #3548 - these should both be fixed together
    • (otherwise behaviour is different is coord(s) are circular or not)
  • analysis.calculus._curl_subtract
    • used in the production of curl cubes
    • original dimensions are retained in the output curl cubes, so no unusual consequences expected if the output cubes now include AV's/CM's
  • analysis._regrid.CurvilinearRegridder
  • analysis.trajectory.UnstructuredNearestNeigbourRegridder, also .__call__ method ❌
  • analysis._grid_angles.rotate_grid_vectors
    • original dimensions are retained in the output cubes, so no unusual consequences expected in the output cubes now include AV's/CM's
  • analysis._grid_angles.rotate_winds
    • see points on analysis._grid_angles.rotate_grid_vectors
  • experimental.stratify.relevel

@trexfeathers
Copy link
Contributor

Thanks @stephenworsley, LGTM ✔

@trexfeathers trexfeathers merged commit 7c648f8 into SciTools:master Nov 20, 2019
"""
all_coords = [cube.coords() for cube in cubes]
if object_get is None:
Copy link
Member

@pp-mo pp-mo Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a public function, so you really need to extend the docstring to cover the changes.
The new 'object_get' keyword is undocumented.
It might also be desirable to rename some things here, since e.g. "coords" is no longer always what it says.

Another way is to create an enhanced, private inner routine + leave the public one as a thin wrapper for coords alone.
Given the naming of the function itself, that might actually be preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #3558 to address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants