IMPRO-1814. Add sig phase mask #1356

MoseleyS · 2020-10-20T08:13:22Z

IMPRO-1814

Updates SnowFraction plugin to handle masked input data as divide-by-zero gives a different behaviour (returns masked-True rather than np.nan).

Adds SignificantPhaseMask plugin and CLI (phase-mask) to convert a snow-fraction field into a mask for one of rain, sleet or snow.

Testing:

Ran tests and they passed OK
Added new tests for the new feature(s)

- The acceptance test for snow-fraction has masked data as inputs (entire mask is false) which results in a masked output (masked where divide-by-zero occurs, no infilling) - Masks now handled such that any input masks are preserved (mismatches are additive) and divide-by-zero points are found and filled in as intended. - This changes the KGO!

- So long as the input snow-fraction field has valid data at all points, it will be used, even if it is a masked-array - Added KGO (which has a mask - all-False)

codecov · 2020-10-20T09:39:03Z

Codecov Report

Merging #1356 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
+ Coverage   96.10%   96.18%   +0.08%     
==========================================
  Files          84       85       +1     
  Lines        7212     7285      +73     
==========================================
+ Hits         6931     7007      +76     
+ Misses        281      278       -3

Impacted Files	Coverage Δ
...ychrometric_calculations/significant_phase_mask.py	`100.00% <100.00%> (ø)`
improver/calibration/reliability_calibration.py	`100.00% <0.00%> (ø)`
improver/utilities/ancillary_creation.py	`98.30% <0.00%> (+3.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bb341f...c3eca25. Read the comment docs.

bayliffe

Thanks for this Stephen. I think I am mostly confused about what you envisage as the behaviour for masked snow/rain/sleet inputs. Other than that I have just a few comments.

improver/snow_fraction.py

improver/psychrometric_calculations/significant_phase_mask.py

improver_tests/snow_fraction/test_SnowFraction.py

...er_tests/psychrometric_calculations/test_significant_phase_mask/test_SignificantPhaseMask.py

- Improves doc-string - Simplifies handling of masks - Improves test

- Removes handling of masked data in snow-fraction plugin as there is no use-case for this. - Snow-fraction acceptance test now checks that inputs can be in either order. - Simplifies test for masked data in significant_phase_mask.py

- Removes handling of masked data in snow-fraction plugin as there is no use-case for this. - Snow-fraction acceptance test now checks that inputs can be in either order. - Simplifies test for masked data in significant_phase_mask.py - Adds model-id-attr option to allow this attribute to be passed to the output cubes

bayliffe

Thanks for the changes Stephen. It is clearer to my mind what is going on now. Two tiny comments.

improver/snow_fraction.py

- Updates doc-string - Suppresses expected warning messages

improver/cli/phase_mask.py

cgsandford · 2020-10-22T08:58:21Z

improver/cli/phase_mask.py

+        model_id_attr (str):
+            Name of the attribute used to identify the source model for
+            blending.
+    """


Why is this needed? Unless this mask is blended between models, it doesn't need this attribute.

I'm not completely convinced that it is, but we will eventually be blending engluk rain/sleet/snow data with enukx data and the blending will need this attribute to know what to do. Strictly, we don't need it on the snow-fraction or phase-mask data, but it does correctly describe them, I think.

We'll be blending the actual diagnostic data - but those fields should have a model ID attribute already present on the probability of precipitation field, which is where they should inherit that metadata from.

I don't think this actively hurts, but in general I'd rather keep the metadata minimal. This attribute is explicitly not in the IMPROVER standard for level 3 products (see @bjwheltor 's latest metadata audit), and we only keep it after level 1 because the blending plugin needs some way to distinguish the inputs. (When we use EC data we may need to change this anyway, since as I understand it they use a coordinate rather than an attribute to identify the input model.)

cgsandford · 2020-10-22T08:58:48Z

improver/cli/snow_fraction.py

+        model_id_attr (str):
+            Name of the attribute used to identify the source model for
+            blending.


Why is this needed?

See previous answer.

improver/cli/snow_fraction.py

cgsandford · 2020-10-22T09:05:48Z

improver/snow_fraction.py

+        with np.errstate(divide="ignore", invalid="ignore"):
+            snow_fraction = self.snow.data / (self.rain.data + self.snow.data)


You expect this to produce np.nan where there is no precipitation. How does the SignificantPhaseMask plugin handle this?

It doesn't have to. The snow-fraction plugin handles these by interpolating to the NaN points from nearby valid points.

In which case, is there a test for the snow fraction plugin to ensure it does that please?

On discussion - yes there is, it just wasn't immediately clear from the docstring. Stephen is going to update the docstring to specify that it tests a point with no precip - and the number that comes out is not masked or nan.

cgsandford · 2020-10-22T09:07:08Z

...er_tests/psychrometric_calculations/test_significant_phase_mask/test_SignificantPhaseMask.py

+        NotImplementedError, match="SignificantPhaseMask cannot handle masked data"
+    ):
+        SignificantPhaseMask()(input_cube, "snow")
+


Your snow fraction field has no mask, but will contain np.nan where there is no precipitation. You need a test for this.

I don't think I do. See previous answer.

...er_tests/psychrometric_calculations/test_significant_phase_mask/test_SignificantPhaseMask.py

improver_tests/snow_fraction/test_SnowFraction.py

improver_tests/acceptance/SHA256SUMS

- Updates doc-string - Moves expected data to be a global variable (as it is used twice) - Extends test doc-string to make it more explicit.

cgsandford

Just would like to be sure no np.nans or masks, so could you add snow fraction unit tests to check this please?

- Updates doc-string

benfitzpatrick · 2020-10-22T10:59:26Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- improver_tests/acceptance/test_phase_mask.py  2
- improver/cli/phase_mask.py  1
- improver/psychrometric_calculations/significant_phase_mask.py  5
- improver_tests/snow_fraction/test_SnowFraction.py  4
- improver_tests/psychrometric_calculations/test_significant_phase_mask/test_SignificantPhaseMask.py  6

See the complete overview on Codacy

* Adds SignificantPhaseMask plugin and tests * Adds SignificantPhaseMask CLI and tests * SnowFraction handles masked-data - The acceptance test for snow-fraction has masked data as inputs (entire mask is false) which results in a masked output (masked where divide-by-zero occurs, no infilling) - Masks now handled such that any input masks are preserved (mismatches are additive) and divide-by-zero points are found and filled in as intended. - This changes the KGO! * SignificantPhaseMask now handles masked arrays if all(False) - So long as the input snow-fraction field has valid data at all points, it will be used, even if it is a masked-array - Added KGO (which has a mask - all-False) * Addresses Codacy errors * Attepts to fix Sphinx error * Reduces duplication in tests with more parametrization. * Attempts to fix Sphinx error * Black * Adds test for invalid phase name request. * First review - Improves doc-string - Simplifies handling of masks - Improves test * Black again. * First review - Removes handling of masked data in snow-fraction plugin as there is no use-case for this. - Snow-fraction acceptance test now checks that inputs can be in either order. - Simplifies test for masked data in significant_phase_mask.py * First review - Removes handling of masked data in snow-fraction plugin as there is no use-case for this. - Snow-fraction acceptance test now checks that inputs can be in either order. - Simplifies test for masked data in significant_phase_mask.py - Adds model-id-attr option to allow this attribute to be passed to the output cubes * Black * First review - Updates doc-string - Suppresses expected warning messages * Second review - Updates doc-string - Moves expected data to be a global variable (as it is used twice) - Extends test doc-string to make it more explicit. * Second review - Updates doc-string

MoseleyS added 4 commits October 19, 2020 11:27

Adds SignificantPhaseMask plugin and tests

322478d

Adds SignificantPhaseMask CLI and tests

30236c7

SignificantPhaseMask now handles masked arrays if all(False)

1963dd6

- So long as the input snow-fraction field has valid data at all points, it will be used, even if it is a masked-array - Added KGO (which has a mask - all-False)

MoseleyS self-assigned this Oct 20, 2020

MoseleyS added 4 commits October 20, 2020 09:30

Addresses Codacy errors

0aa1afd

Attepts to fix Sphinx error

a636066

Reduces duplication in tests with more parametrization.

cff4841

Attempts to fix Sphinx error

c6fbb7e

MoseleyS added 2 commits October 20, 2020 10:42

Black

335036d

Adds test for invalid phase name request.

9dccb9f

bayliffe requested changes Oct 21, 2020

View reviewed changes

MoseleyS added 5 commits October 21, 2020 11:29

First review

011f86d

- Improves doc-string - Simplifies handling of masks - Improves test

Black again.

e142152

First review

0b8c45c

- Removes handling of masked data in snow-fraction plugin as there is no use-case for this. - Snow-fraction acceptance test now checks that inputs can be in either order. - Simplifies test for masked data in significant_phase_mask.py

Black

1ed9d44

bayliffe requested changes Oct 22, 2020

View reviewed changes

improver/snow_fraction.py Show resolved Hide resolved

improver/snow_fraction.py Outdated Show resolved Hide resolved

First review

680a375

- Updates doc-string - Suppresses expected warning messages

bayliffe previously approved these changes Oct 22, 2020

View reviewed changes

cgsandford requested changes Oct 22, 2020

View reviewed changes

Second review

fab1593

- Updates doc-string - Moves expected data to be a global variable (as it is used twice) - Extends test doc-string to make it more explicit.

MoseleyS dismissed bayliffe’s stale review via fab1593 October 22, 2020 10:00

cgsandford reviewed Oct 22, 2020

View reviewed changes

Second review

c3eca25

- Updates doc-string

cgsandford approved these changes Oct 23, 2020

View reviewed changes

MoseleyS merged commit 791ff96 into metoppv:master Oct 23, 2020

MoseleyS deleted the impro-1814-sig-phase-mask branch October 23, 2020 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IMPRO-1814. Add sig phase mask #1356

IMPRO-1814. Add sig phase mask #1356

MoseleyS commented Oct 20, 2020

codecov bot commented Oct 20, 2020 •

edited

Loading

bayliffe left a comment

bayliffe left a comment

cgsandford Oct 22, 2020

MoseleyS Oct 22, 2020

cgsandford Oct 22, 2020

cgsandford Oct 22, 2020

MoseleyS Oct 22, 2020

cgsandford Oct 22, 2020

MoseleyS Oct 22, 2020

cgsandford Oct 22, 2020

cgsandford Oct 22, 2020

MoseleyS Oct 22, 2020

cgsandford Oct 22, 2020

MoseleyS Oct 22, 2020

cgsandford left a comment

benfitzpatrick commented Oct 22, 2020

		with np.errstate(divide="ignore", invalid="ignore"):
		snow_fraction = self.snow.data / (self.rain.data + self.snow.data)

IMPRO-1814. Add sig phase mask #1356

IMPRO-1814. Add sig phase mask #1356

Conversation

MoseleyS commented Oct 20, 2020

codecov bot commented Oct 20, 2020 • edited Loading

Codecov Report

bayliffe left a comment

Choose a reason for hiding this comment

bayliffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgsandford left a comment

Choose a reason for hiding this comment

benfitzpatrick commented Oct 22, 2020

codecov bot commented Oct 20, 2020 •

edited

Loading