Refactor CubeCombiner #1383
Refactor CubeCombiner #1383
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1383 +/- ##
==========================================
+ Coverage 96.32% 96.34% +0.02%
==========================================
Files 89 89
Lines 7583 7586 +3
==========================================
+ Hits 7304 7309 +5
+ Misses 279 277 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks @cgsandford 👍
This seems fine. I've added some very minor comments.
improver/cube_combiner.py
Outdated
| match the dimensions, in order, of the first cube in the list | ||
|
|
||
| Args: | ||
| cube_list: (iris.cube.CubeList) |
There was a problem hiding this comment.
The colon isn't required here and I think this could be a list as well (similar to the amendment that you've made to the _combine_cube_data method):
| cube_list: (iris.cube.CubeList) | |
| cube_list (iris.cube.CubeList or list) |
| """ | ||
| if len(cube_list) < 2: | ||
| msg = "Expecting 2 or more cubes in cube_list" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
As CodeCov highlights, this exception could do with a unit test to cover it.
improver/cube_combiner.py
Outdated
|
|
||
| Raises: | ||
| ValueError: if the operation results in an escalated datatype |
There was a problem hiding this comment.
I think this should actually be:
| ValueError: if the operation results in an escalated datatype | |
| TypeError: if the operation results in an escalated datatype |
… Need to sort unit tests and then make code nicer.
…nsistent with use case and CLI interface.
…idying up but all tests pass.
cgsandford
force-pushed
the
refactor_combine
branch
from
20ca58c to
c85478c
Compare
improver/cube_combiner.py
Outdated
| self.normalise = operation == "mean" | ||
|
|
||
| @staticmethod | ||
| def _check_dimensions_match(cube_list, comparators=[eq]): | ||
| """ | ||
| Check all coordinate dimensions on the input cubes are equal or broadcastable |
There was a problem hiding this comment.
Ability to broadcast has gone, so probably need to amend this doc-string?
improver/cube_combiner.py
Outdated
|
|
||
| Args: | ||
| cube_list: (iris.cube.CubeList) | ||
| cube_list (iris.cube.CubeList or list) |
There was a problem hiding this comment.
Because CubeList is a sub-class of list, and you don't need any of the added functionality, I think you can just specify list here.
There was a problem hiding this comment.
Have changed this in most of the places it occurs - there's nowhere in this module that uses merging or other special CubeList functionality.
| msg = ( | ||
| r"Operation add on types \{dtype\(\'.*\'\)\} results in " | ||
| r"float64 data which cannot be safely coerced to float32" | ||
| ) | ||
| msg = "Operation .* results in float64 data" |
There was a problem hiding this comment.
Why change this? The behaviour hasn't changed has it?
There was a problem hiding this comment.
The behaviour has very slightly changed, in that this error will now have eg np.add instead of add in the string.
In terms of why I cut it down though. I saw this done a while ago in some of the pytests and asked @tjtg about it. The answer was broadly: we don't really need to verify the whole error string, as we might have legitimate reasons for changing the details in future. We just want to check enough to identify that it's the error we wanted to raise, and that any important components are there. In this case I judged that the important components are the fact that we've escalated to 64-bit precision, so I kept that.
* Factored multiplication case out of CubeCombiner. All CLI tests pass. Need to sort unit tests and then make code nicer. * Split existing unit tests between plugins and they pass. * Reduced "broadcast_to_coords" to "broadcast_to_threshold" boolean, consistent with use case and CLI interface. * Simplified setting up of coords for broadcast to use threshold only. * Factored out broadcasting into CubeMultiplier. Coord checking needs tidying up but all tests pass. * Removed unused method. * Pythonified coordinate checking. * Removed "self.operation" and summarised use more clearly. * Addressed comments from first review. * Response to second review.
Single responsibility principle. Splits
CubeCombinerinto two plugins: a "parent" plugin to apply linear operators, and aCubeMultiplierplugin to do the additional work of broadcasting to threshold coordinates in the "multiply" case.Unit tests have not been changed or extended - existing tests have been copied for
CubeMultiplierwith updated calls. This, and the passing of the "combine" CLI tests, show that functionality is unchanged with this refactor.Testing: