Add function which normalises input cubes according to a reference #1919

brhooper · 2023-06-27T15:27:43Z

Addresses https://github.com/metoppv/mo-blue-team/issues/520
Acceptance test data: metoppv/improver_test_data#20

Description
Adds plugin and CLI which takes a cubelist and a reference cube as inputs and adjusts the data in the input cubelist so that the sum total of this data is equal to the data in reference cube. Also adds unit tests for the new function.

Testing:

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

…cast. Add unit tests for new function.

codecov · 2023-06-27T15:36:33Z

Codecov Report

Merging #1919 (efefeab) into master (8b218e0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1919   +/-   ##
=======================================
  Coverage   98.36%   98.37%           
=======================================
  Files         122      122           
  Lines       11643    11707   +64     
=======================================
+ Hits        11453    11517   +64     
  Misses        190      190

Impacted Files	Coverage Δ
...semble_copula_coupling/ensemble_copula_coupling.py	`99.21% <100.00%> (+<0.01%)`	⬆️
...prover/utilities/forecast_reference_enforcement.py	`100.00% <100.00%> (ø)`

bayliffe

Thanks for taking this on Ben. I think there is quite a lot more to do yet.

Can you explain how you envisage multi-percentile data being processed by this, e.g. so that the 20th, 50th, and 80th percentiles of the cubelist diagnostics are all normalised relative to the matching percentiles in the reference cube?
How will it handle the case in which these percentiles don't match between cubelist diagnostics and the reference cube?
We need a way to call this function / plugin (whatever it ends up being), so it needs a CLI.
Can you outline the chain of calls in the suite that will be required, including to this code, to normalise the percentiles and regenerate the probabilities?
Is there overlap with the consistency code you've recently written? i.e. could consistency either be capping a value, or normalising a value? (I've not looked at that code, but just a thought).

…o more sensible location.

…s checksums.

…tests. Move unit tests file to correct directory.

Katie-Howard

I've looked through all these changes, and they seem generally fine to me. There are just a few minor comments that I have made on the code. It all seems to work fine, and the new unit tests/acceptance tests also run through without any problems. I've had a little look at the new acceptance test data and that too looks ok 👍

improver/cli/enforce_consistent_forecasts.py

improver/cli/normalise_to_reference.py

improver_tests/utilities/test_normalise_to_reference.py

Katie-Howard

I've looked through the changes and I'm happy with them now, thanks 👍

…ormalise_to_reference

…responding unit tests. Change to acceptance test for changed CLI.

Katie-Howard · 2023-07-13T14:40:21Z

I've looked again and the acceptance tests fail for the checksums and wx-modal. I think it's because of the weather symbols work that has been merged. So if you update your code it should work hopefully.

…rmalise_to_reference

bayliffe

A couple of comments and now I can't read anymore (migraine aura's are trippy). Bailing.

bayliffe · 2023-07-13T14:17:24Z

improver/utilities/forecast_reference_enforcement.py

+
+    desired_cubes = iris.cube.CubeList()
+    other_cubes = iris.cube.CubeList()
+


I wonder whether it is tidier to make this function return all the cubes as desired_cubes if cube_names == None. It would remove the logic in the CLI in relation to return_name, and allow this function to be called in other places where the status of return_name is similarly unknown.

bayliffe · 2023-07-13T14:48:21Z

improver/cli/normalise_to_reference.py


-    return normalise_to_reference(cubes, reference, ignore_zero_total)
+    cubes = cubes[0]


This should not be here.

I think I understand why this was put here, but it's not doing what you want when called from the command line; actually the behaviour of the acceptance tests is rather weird. From the command line, calling this CLI with multiple cubes, the cubes variable becomes something like this:

( [ <iris 'Cube' of probability_of_lwe_thickness_of_precipitation_amount_above_threshold / (1) (lwe_thickness_of_precipitation_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)> ], [ <iris 'Cube' of probability_of_thickness_of_rainfall_amount_above_threshold / (1) (thickness_of_rainfall_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)> ], [ <iris 'Cube' of probability_of_lwe_thickness_of_sleetfall_amount_above_threshold / (1) (lwe_thickness_of_sleetfall_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)> ], [ <iris 'Cube' of probability_of_lwe_thickness_of_snowfall_amount_above_threshold / (1) (lwe_thickness_of_snowfall_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)> ] )

Meaning that your cubes = cubes[0] line just gives you:

cubes = [ <iris 'Cube' of probability_of_lwe_thickness_of_precipitation_amount_above_threshold / (1) (lwe_thickness_of_precipitation_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)> ],

However, your acceptance tests create a single list wrapped in a tuple:

( [ <iris 'Cube' of probability_of_lwe_thickness_of_precipitation_amount_above_threshold / (1) (lwe_thickness_of_precipitation_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)>, <iris 'Cube' of probability_of_thickness_of_rainfall_amount_above_threshold / (1) (thickness_of_rainfall_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)>, <iris 'Cube' of probability_of_lwe_thickness_of_sleetfall_amount_above_threshold / (1) (lwe_thickness_of_sleetfall_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)>, <iris 'Cube' of probability_of_lwe_thickness_of_snowfall_amount_above_threshold / (1) (lwe_thickness_of_snowfall_amount: 27; projection_y_coordinate: 970; projection_x_coordinate: 1042)> ] )

Hence your command yields a simple list that can be iterated over.
I have absolutely no idea why calling the CLI from the acceptance test is behaving differently to the command line and it perturbs me. However, the solution is to use the flatten utility which is used in some other CLIs that accept a cubelist. This will enable the acceptance tests and command line calls to behave in the same way.

from improver.utilities.flatten import flatten cubes = flatten(cubes)

Thanks for this suggestion @bayliffe, I've implemented it and everything is working again.

improver/cli/normalise_to_reference.py

MoseleyS · 2023-07-13T12:51:22Z

improver/utilities/forecast_reference_enforcement.py

+
+    # check cube compatibility
+    reference_dim_coords = reference.coords(dim_coords=True)
+    n_dims_mismatch = False


I think you can replace the next ~40 lines with

for cube in cubes: cube = check_cube_coordinates(reference, cube)

I'm not sure that I can, because that chunk of code looks at the names of the dimension coordinates and then compares each coordinate between cubes, whereas I think check_cube_coordinates() only checks that the same dimension coordinates exist on each cube.

I do agree that this section of code could be tidied up, though, but my brain is currently failing me on how to do it.

I've not made any change here for now, though I'm happy to revisit it. I think as any future change wouldn't affect the input/output for the plugin that it can safely be made later without changing the CLI.

improver/utilities/forecast_reference_enforcement.py

improver_tests/acceptance/test_normalise_to_reference.py

improver_tests/utilities/test_split_cubes_by_name.py

improver_tests/utilities/test_normalise_to_reference.py

MoseleyS

Just one more thing...

MoseleyS · 2023-07-14T10:59:55Z

improver_tests/acceptance/test_normalise_to_reference.py

+
+@pytest.mark.parametrize(
+    "forecast_type,ignore_zero_total",
+    (("probability", False), ("percentile", False), ("percentile", True)),


As ("probability", True) isn't included, I don't think the probability/kgo.nc is used.

The probability inputs don't contain any instances where the total of the inputs is zero but the reference is non-zero. Due to this the CLI can happily run with ignore_zero_total=False (the default), so I don't think that a change is required here.

Later on if forecast_type == "probability" is checked before checking ignore_zero_total, so for probability inputs the test runs regardless of the value of ignore_zero_total

Sorry - I had misread the if..elif..else block

MoseleyS · 2023-07-14T11:16:20Z

improver_tests/acceptance/test_normalise_to_reference.py

+
+@pytest.mark.parametrize(
+    "forecast_type,ignore_zero_total",
+    (("probability", False), ("percentile", False), ("percentile", True)),


Sorry - I had misread the if..elif..else block

Katie-Howard

These changes look good to me.

bayliffe

I've not reviewed the unit tests, but you seem to have ample reviewers, so I think that's okay. I'm happy with this, thanks Ben.

Merging, along with the acceptance test data.

* upstream/master: Fix to the wind vertical displacement adjustment implementation (metoppv#1927) Add function which normalises input cubes according to a reference (metoppv#1919) Skip ECC bounds usage when converting probabilities to percentiles (metoppv#1926) Add CLIs to support rescaling of the forecast based on altitude difference (metoppv#1917) Changes to the modal code to increase the percentage to 30% and change the groupings to provide a more representative daily summary symbol. (metoppv#1925) Add plugins to support rescaling of the forecast based on altitude difference (metoppv#1916) Support conversion from percentiles to probabilities (metoppv#1924) Correct handling of reference time in weather_code plugin (metoppv#1920) Add CLI for clipping cubes (metoppv#1918) Update cbh ecc name (metoppv#1922) Updates Broadcast and expand_bounds in Combine Plugin (metoppv#1914) Mobt515 cloud base height spot extraction (metoppv#1911) MOBT-494: Cube title setting in weather symbol code (metoppv#1912) MOBT512-masking percentiles for cloud base height (metoppv#1908) Mobt 496 enforce forecast between references (metoppv#1907)

…etoppv#1919) * Add new function to normalisetotal of input cubes to a reference forecast. Add unit tests for new function. * Add docstrings. * Formatting. * Minor change to existing code. * Add functionality to convert between percentile sets. Move function to more sensible location. * Fixes to plugin. Adds unit tests, a CLI, and acceptance tests. Updates checksums. * Adds CLI and acceptance tests. * Updates formatting. Removes unnecessary module imports. * Update formatting of module imports. * Remove obsolete file. * Update checksums * Add handling of probability forecasts. Add probability forecast unit tests. Move unit tests file to correct directory. * Correct formatting. Remove obsolete module import. * Minor changes following review. * Correct typo. * Change to CLI to make compatible with suite. Add new function and corresponding unit tests. Change to acceptance test for changed CLI. * Changes following review. Restrict CLI to only output exactly one cube.

brhooper added 3 commits June 27, 2023 15:20

Add new function to normalisetotal of input cubes to a reference fore…

5a9ea57

…cast. Add unit tests for new function.

Add docstrings.

1417326

Formatting.

7f22742

brhooper changed the title ~~Mobt 520 normalise to reference~~ Add function which normalises input cubes according to a reference Jun 27, 2023

Minor change to existing code.

55086ab

bayliffe reviewed Jun 28, 2023

View reviewed changes

bayliffe assigned brhooper Jun 28, 2023

brhooper added 6 commits June 29, 2023 07:19

Add functionality to convert between percentile sets. Move function t…

2328e18

…o more sensible location.

Fixes to plugin. Adds unit tests, a CLI, and acceptance tests. Update…

1e0436b

…s checksums.

Adds CLI and acceptance tests.

08ad694

Updates formatting. Removes unnecessary module imports.

44e0814

Update formatting of module imports.

8e54dee

Remove obsolete file.

bc1d459

brhooper mentioned this pull request Jul 6, 2023

Mobt 520 normalise to reference metoppv/improver_test_data#20

Merged

brhooper added 3 commits July 6, 2023 14:33

Update checksums

339a251

Add handling of probability forecasts. Add probability forecast unit …

d5e80f0

…tests. Move unit tests file to correct directory.

Correct formatting. Remove obsolete module import.

a0df265

Katie-Howard requested changes Jul 11, 2023

View reviewed changes

brhooper added 2 commits July 11, 2023 15:40

Minor changes following review.

0eb8bf6

Correct typo.

5b34456

brhooper assigned Katie-Howard and unassigned brhooper Jul 11, 2023

Katie-Howard previously approved these changes Jul 12, 2023

View reviewed changes

brhooper added 2 commits July 13, 2023 07:24

Merge branch 'master' of github.com:brhooper/improver into mobt_520_n…

d2552f4

…ormalise_to_reference

Change to CLI to make compatible with suite. Add new function and cor…

17faad3

…responding unit tests. Change to acceptance test for changed CLI.

brhooper dismissed Katie-Howard’s stale review via 17faad3 July 13, 2023 11:13

Merge branch 'master' of github.com:metoppv/improver into mobt_520_no…

55703c7

…rmalise_to_reference

bayliffe requested changes Jul 13, 2023

View reviewed changes

MoseleyS requested changes Jul 13, 2023

View reviewed changes

Changes following review. Restrict CLI to only output exactly one cube.

efefeab

MoseleyS requested changes Jul 14, 2023

View reviewed changes

MoseleyS approved these changes Jul 14, 2023

View reviewed changes

Katie-Howard approved these changes Jul 14, 2023

View reviewed changes

bayliffe approved these changes Jul 14, 2023

View reviewed changes

bayliffe merged commit 523ae6f into metoppv:master Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function which normalises input cubes according to a reference #1919

Add function which normalises input cubes according to a reference #1919

brhooper commented Jun 27, 2023 •

edited

Loading

codecov bot commented Jun 27, 2023 •

edited

Loading

bayliffe left a comment

Katie-Howard left a comment

Katie-Howard left a comment

Katie-Howard commented Jul 13, 2023

bayliffe left a comment

bayliffe Jul 13, 2023

brhooper Jul 14, 2023

bayliffe Jul 13, 2023

MoseleyS Jul 13, 2023

bayliffe Jul 14, 2023

brhooper Jul 14, 2023

MoseleyS Jul 13, 2023

brhooper Jul 14, 2023

MoseleyS Jul 14, 2023

MoseleyS left a comment

MoseleyS Jul 14, 2023

brhooper Jul 14, 2023 •

edited

Loading

MoseleyS Jul 14, 2023

MoseleyS Jul 14, 2023

Katie-Howard left a comment

bayliffe left a comment


		desired_cubes = iris.cube.CubeList()
		other_cubes = iris.cube.CubeList()


		return normalise_to_reference(cubes, reference, ignore_zero_total)
		cubes = cubes[0]

Add function which normalises input cubes according to a reference #1919

Add function which normalises input cubes according to a reference #1919

Conversation

brhooper commented Jun 27, 2023 • edited Loading

codecov bot commented Jun 27, 2023 • edited Loading

Codecov Report

bayliffe left a comment

Choose a reason for hiding this comment

Katie-Howard left a comment

Choose a reason for hiding this comment

Katie-Howard left a comment

Choose a reason for hiding this comment

Katie-Howard commented Jul 13, 2023

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

MoseleyS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brhooper Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Katie-Howard left a comment

Choose a reason for hiding this comment

bayliffe left a comment

Choose a reason for hiding this comment

brhooper commented Jun 27, 2023 •

edited

Loading

codecov bot commented Jun 27, 2023 •

edited

Loading

brhooper Jul 14, 2023 •

edited

Loading