-
Notifications
You must be signed in to change notification settings - Fork 284
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
Equalise cubes #6257
base: main
Are you sure you want to change the base?
Equalise cubes #6257
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6257 +/- ##
==========================================
- Coverage 89.83% 89.83% -0.01%
==========================================
Files 88 88
Lines 23315 23380 +65
Branches 4338 4356 +18
==========================================
+ Hits 20945 21003 +58
- Misses 1644 1649 +5
- Partials 726 728 +2 ☔ View full report in Codecov by Sentry. |
Status updateI think this is now good enough to consider as it is, though I'm still anticipating other useful features could be added. "Grouping" implementationAs noted here in the original issue - section "Grouping of input cubes ?", The notes there describe the problem of trying to rationalise the different ways in which merge and concatenate do this "grouping"
Currently included options :
Other possible, future optionsSee also list in original issue
We can also, in future, usefully include this in the "combine_cubes" operation and "COMBINE_POLICY" / "LOAD_POLICY" settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a couple things, mostly about documentation. This will also want a whatsnew describing the new function.
# Apply operations to the groups : in-place modifications on the cubes | ||
for group_cubes in cube_group_cubes: | ||
for op in equalisation_ops: | ||
op(group_cubes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the default behaviour, when all kwargs are False
, does nothing to the cubes. Would it be helpful to raise a warning in the case where all of these are False
? I can imagine a user having an expectation that this might do something without having to alter any of the kwargs.
# Snapshot the cube metadata elements which we use to identify input groups | ||
# TODO: we might want to sanitise practically comparable types here ? | ||
# (e.g. large object arrays ??) | ||
cube_grouping_keys = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be renamed cube_grouping_dicts
. This is a list of dictionarys rather than a list of keys.
# TODO: might something nasty happen here if attributes contain weird stuff ?? | ||
cube_group_keys = [] | ||
cube_group_cubes = [] | ||
for cube, cube_group_key in zip(cubes, cube_grouping_keys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above, the name cube_group_key
is misleading as I believe this object should be a dictionary rather than a key.
:class:`~iris.cube.CubeList` | ||
A CubeList containing the original input cubes, ready for merge or concatenate | ||
operations. The cubes are possibly modified (in-place), and possibly in a | ||
different order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be edited a little so that it doesn't suggest that the cubes might be modified in place in a different order. Something like "..., and are possibly returned in a different order".
from iris.common.metadata import CubeMetadata | ||
from iris.cube import CubeList | ||
|
||
if unify_names or apply_all: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is unify_names
the correct name for this operation? Compared to unify_time_units
, this operation works quite differently. Only removing information rather than editing it. unify_names
might suggest that equivalent standard_name
s are made the same for example. This may also cause us problems if we ever wanted to add that sort of functionality in the future. A better name might be standardise_names
or trim_names
.
scramble_inds = rng.permutation(n_inputs) | ||
inputs_array = inputs_array[scramble_inds] | ||
# Modify input list **BUT N.B. IN PLACE** | ||
inputs[0:] = inputs_array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could [0:]
not be replaced by [:]
?
Closes #6248