Refactored internals of iris.analysis.maths #888
Conversation
|
After a quick overview this looks promising - thanks @shoyer. 😄 (For future reference, putting the code formatting and docstring changes in a separate PR would make it easier to see the other changes.) |
There was a problem hiding this comment.
By postponing this check until _binary_op_common() the error from not supplying a Cube can change. For example, _assert_matching_units() might raise an AttributeError.
|
An excellent PR - both the initial description and the code. Thanks again @shoyer. 😄 The only functional change I spotted was the TypeError issue when the |
|
OK, new patch set is up. |
lib/iris/analysis/maths.py
Outdated
There was a problem hiding this comment.
Step ahead of yourself here - we don't have that alias. Yet... 😉
There was a problem hiding this comment.
This is actually the original error message in this file (I didn't write this)... but fixed anyways!
The goal here was to reduce redundant code and consolidate functionality, to facilitate future extensions of iris.analysis operators. For example, I would like to be able to easily define new cube operations like `>`, `<`, `>=`, `<=`, `&`, `|`, `^` and `%` that are analogous to the standard numpy operations. By putting binary operations that involving broadcasting a cube and another object together, it should also be easier to extend the cube broadcasting rules. This change includes adjusted docstrings to reflect current functionality and removal of code that wasn't doing anything (as caught by pylint). I also fixed unary math operations (abs, log, exp, etc.) to actually calculate values in place when `in_place=True`. None of the public facing API is changed. That will come next :). The refactoring did reveal a few places where broadcasting rules differ for no clear reason between add/subtract and multiply/divide. Arguably, the special logic for add/subtract broadcasting should also be consolidated into `_binary_op_common` so that it used for multiply/divide as well. Also, there is a case for exposing public interfaces to `_math_op_common` and `_binary_op_common` to facilitate writing new cube operations, just like how `iris.analaysis.Aggregator` facilitates writing new cube aggregations.
Per PR discussion.
|
Oops, I seem to have messed up the history a little bit because of a For now (see this latest patchset), I decided to move the TypeError tests to the More broadly, I understand the value in having a uniform directory structure for tests, but I'm not sure it really makes sense to have a separate file for each top-level function or class. I plan on adding about 15 new functions to |
That's fine - thanks.
A lot of short files is fine by me. It's large messy blobs that cause me concern. |
|
OK - I think this is good to go. 👍 I'll leave it a little while to give @pelson a chance to comment and if it's still all clear then merge. |
…erations Refactored internals of iris.analysis.maths
|
🎉 😀 |
3 participants
The goal here was to reduce redundant code and consolidate functionality, to facilitate future extensions of iris.analysis operators.
For example, I would like to be able to easily define new cube operations like
>,<,>=,<=,&,|,^and%that are analogous to the standard numpy operations.By putting binary operations that involving broadcasting a cube and another object together, it should also be easier to extend the cube broadcasting rules.
This change includes adjusted docstrings to reflect current functionality and removal of code that wasn't doing anything (as caught by pylint). I also fixed unary math operations (abs, log, exp, etc.) to actually calculate values in place when
in_place=True. None of the public facing API is changed. That will come next :).The refactoring did reveal a few places where broadcasting rules differ for no clear reason between add/subtract and multiply/divide. Arguably, the special logic for add/subtract broadcasting should also be consolidated into
_binary_op_commonso that it used for multiply/divide as well. Also, there is a case for exposing public interfaces to_math_op_commonand_binary_op_commonto facilitate writing new cube operations, just like howiris.analaysis.Aggregatorfacilitates writing new cube aggregations.