-
Notifications
You must be signed in to change notification settings - Fork 100
EPPT-2991 Add _zero_mask_values method to CubeCombiner with preserve_… #2286
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
base: master
Are you sure you want to change the base?
EPPT-2991 Add _zero_mask_values method to CubeCombiner with preserve_… #2286
Conversation
98bb47c to
dc565d3
Compare
|
CI failure addressed in #2287 |
dc565d3 to
4113a03
Compare
| Warning: | ||
| The masked_add function will retain the mask wherever the input masks | ||
| are overlapping. | ||
|
|
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 added these warnings because I did some digging in to the masked_add function to see whether it was a distinct method that still requires it's own function or whether it can be handled under the _zero_mask_values method when preserve_mask_values=False. The methods do differ but I'm not entirely convinced this is intentional.
I suspect that this overlapping issue wasn't a concern in the original work (masking land and sea) because when masks are created to distinguish land and sea they are most likely created from the same source coastline (so overlaps should not happen). That said I'm struggling to think of a use case for a scenario where we retain only overlapping masks rather than allowing them to be visible as zeros. Should we be raising a task to deprecate this function and use the new method instead or is there a use case I'm unaware of that can be included in the docstrings?
The background to the masked_add function was task EPPT1305 and the work was on behalf of the stability adjusted wind gust wf. According to page 6 of the linked workflow plan that work may ultimately not have been needed.
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.
@Anzerkhan27 Did you have a position on this? I'm primarily wondering if the masked_add behaviour is in use anywhere and/or whether it could/should be deprecated in favour of the new approach which handles masked values on any of the operations.
Anzerkhan27
left a comment
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.
Approve with one minor type hint correction needed. The implementation is clean, well-tested, and maintains backward compatibility with the default preserve_mask_values=True
EPPT-2991 Add _zero_mask_values method to CubeCombiner with preserve_mask_values flag.
Description
During development of https://metoffice.atlassian.net/browse/EPPT-2991 we identified a need for mask handling where the masked data could be treated as zero to enable calculations to take place without the masked area persisting into downstream datasets. Adding the _zero_mask_values method to CubeCombiner enables this behaviour when the preserve_mask_values option is set to False.
Testing:
Duplicated the test_with_mask test to create test_process_add_preserve_mask_true and test_process_add_preserve_mask_false which test the existing behaviour and then the _zero_mask_values behaviour with the
addoperator. Repeated the above for themultiplyoperator.Upon realising that the existing masked_add operator currently retains the mask wherever the input masks are overlapping, added a test for masked_add to demonstrate this behaviour and for the Test_process tests to demonstrate the opposite when _zero_mask_values is used.
CLA