Ensure consistent units of the realization coordinate in RealizationClusterAndMatch - #2414
Ensure consistent units of the realization coordinate in RealizationClusterAndMatch#2414gavinevans wants to merge 4 commits into
Conversation
mo-jbeaver
left a comment
There was a problem hiding this comment.
Added a few comments to the code but the tests all ran successfully.
mo-jbeaver
left a comment
There was a problem hiding this comment.
Thanks for adding the docstring and explaining the unit's context. Happy to approve.
brhooper
left a comment
There was a problem hiding this comment.
Thanks @gavinevans
The code here looks fine, but I don't understand why it is necessary or desirable. Do we understand how we can end up in a situation where the units of forecast cubes do not accurately reflect the actual units of the quantities within?
| contains more realizations than the primary input. | ||
| contains more realizations than the primary input. It also demonstrates that | ||
| input cubes can carry realization units of "unknown" while processing still | ||
| succeeds because output realization units are normalised prior to merging. |
There was a problem hiding this comment.
| succeeds because output realization units are normalised prior to merging. | |
| succeeds because output realization units are unified prior to merging. |
There was a problem hiding this comment.
Corrected. Regarding the reason that this change is necessary, I've put a bit more context in this comment as to why the realization coordinate units are currently mismatching.
There was a problem hiding this comment.
Thanks. Would it make more sense to modify improver/standardise.py (maybe this function
improver/improver/standardise.py
Line 219 in ea14bf0
1?
There was a problem hiding this comment.
Do you mean to enforce that the realization coordinate as a unit of 1 regardless of the input, similar to the way that the time coordinates have their units converted regardless after getting the centralised accepted units for the time coordinates (https://github.com/metoppv/improver/blob/master/improver/metadata/check_datatypes.py#L109)? I'd thought of modifying the standardise CLI/plugin to be able to set the units of a coordinate, but I thought that would be a fairly random addition as setting the units, rather than converting the units from one unit to another would be a bit odd in a lot of situations, so I didn't think this was best placed as a general fix, so I've just fixed it where it matters in this PR. If you're interested in enforcing that realization coordinate has a unit of 1 regardless of the input through standardise, as done for the time coordinates, perhaps that is something that could be done as a generalisation later (i.e. after this PR is merged), so as not to complicate things in the short-term.
Related issues: #2259, #2305, #2332, #2340, #2359, #2361
Description
This PR makes a minor edit to ensure the the realization coordinate units are consistent prior to attempting to merge cubes together from different sources. These forecast sources should have the same units for the realization coordinate, however, this can sometimes not be true, with some forecast sources having a realization coordinate with units of "1" and some forecast sources having a realization coordinate with units of
unknown. To avoid this resulting in merge failures, the units of the realization coordinate is manually set to "1".Testing: