Fix cluster matching for "partial" secondary inputs by gavinevans · Pull Request #2417 · metoppv/improver

gavinevans · 2026-07-24T09:00:04Z

Related issues: #2259, #2305, #2332, #2340, #2359, #2361

Description
This PR consists of 3 commits to address issues with the current clustering logic.

Commit 1: This addresses the primary goal because I noticed that it was possible for secondary inputs (e.g. UKV), that only have enough realizations to cover a subset of the total number of clusters, to match to different clusters at different forecast periods e.g. see the output below. This wasn't the intention but was arising from the fact that the call to RealizationToClusterMatcher was inside the loop over forecast period in _process_partial_realization_inputs. This was unlike the methodology in _process_full_realization_inputs where RealizationToClusterMatcher was called prior to the for loop over forecast period, so that the realization to cluster matching used all forecast periods. This commit therefore makes _process_partial_realization_inputs more like _process_full_realization_inputs, so that secondary inputs with a "partial" set of realizations are matched to the clusters using all the forecast periods.

json.loads(ccube.attributes["secondary_input_realizations_to_clusters"])["uk_det"]
{'10': [{'realization': 3, 'forecast_periods': [3600]}, {'realization': 1, 'forecast_periods': [21600]}], '11': [{'realization': 2, 'forecast_periods': [3600]}], '13': [{'realization': 0, 'forecast_periods': [3600]}], '15': [{'realization': 1, 'forecast_periods': [3600]}], '0': [{'realization': 2, 'forecast_periods': [21600]}], '5': [{'realization': 3, 'forecast_periods': [21600]}], '17': [{'realization': 0, 'forecast_periods': [21600]}]}

Commit 2: This addresses an issue that was found in testing commit 1, where it was possible for a secondary input to have an inconsistent number of realizations at different forecast periods. This led to an inability to merge the secondary inputs across forecast periods in _process_partial_realization_inputs. This hadn't been an issue previously because different "partial" forecast sources were being matched independently at each forecast period.

iris.exceptions.MergeError: failed to merge into a single cube.
  cube.shape differs: (4, 970, 1042) != (3, 970, 1042)

To fix this issue, I've modified the _categorise_secondary_inputs method, which was previously only checking the number of realizations present on the first cube. This has been extended, so that if the number of realizations changes across forecast periods within the secondary input then the secondary input will be truncated, so as to avoid a possible pulsing if a forecast source is missing particular forecast periods.

Commit 3: This commit does some refactoring to centralise some of the code present in _process_partial_realization_inputs and _process_full_realization_inputs and avoid duplication where possible.

Testing:

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

… the secondary forecast source are fewer than the number of clusters.

…izations in the secondary inputs changed over time, so a merge would fail. This has now been handled by curtailing any secondary input if there is a change in the number of realizations.

…nputs and _process_full_realization_inputs.

mo-jbeaver

Added some comments, mostly for areas that might need more explanations. Tests all ran successfully.

mo-jbeaver · 2026-07-27T08:52:32Z

+    Cross-fp avg MSE (new code):
+    - vs cluster A: (4 + 7225)/2 = 3614.5
+    - vs cluster B: (7744 + 25)/2 = 3884.5
+    => new code assigns to cluster A at both fps


Should a small note be added that for MSE, lower is better?

mo-jbeaver · 2026-07-27T08:55:31Z

+    """Partial inputs must assign the same cluster at every lead time.
+
+    A partial secondary input (fewer realizations than n_clusters) was previously
+    matched independently per forecast period, allowing the same realization to be


forecast period (fp) might be a helpful addition

I've added (fp) given that that's used in lots of variable names later on.

mo-jbeaver · 2026-07-27T10:12:53Z

@@ -705,8 +717,31 @@ def _categorise_secondary_inputs(
                )
                continue  # No valid forecast periods after filtering


Would it make more sense to move this section to below the lines:
valid_fp_cube_pairs = [ (fp, cube) for fp, cube in fp_cube_pairs if fp in primary_fps ] valid_fp_cube_pairs = sorted(valid_fp_cube_pairs, key=lambda x: x[0])

Yes, I've moved the if not valid_fp_cube_pairs block up a few lines to be next to where the valid_fp_cube_pairs variable is defined.

mo-jbeaver · 2026-07-27T10:28:55Z

+    fp_6_data = result.extract(iris.Constraint(forecast_period=6 * 3600)).data
+    assert np.allclose(fp_6_data, 106.0, atol=5.0), (
+        "fp=6 should fall back to clustered primary when secondary is truncated"
+    )


Could an additional comment be added here to explain this section.

I've added an additional comment to explain the meaning of the result.

mo-jbeaver · 2026-07-27T10:35:37Z

+    )
+
+
+def test_clusterandmatch_full_input_inconsistent_realization_counts_truncates():


This test seems very similar to the previous one. Could more descriptions be added to both to explain the differences between them.

I've parameterised these tests, as you're right that they were very similar. The unit test description has been extended.

mo-jbeaver · 2026-07-27T10:39:12Z

+        Updates replaced_realizations, cluster_sources, and
+        secondary_input_realizations_to_clusters in-place. Called from the


Maybe mention what these variables are, attributes, dimensions ect.

I've added a bit more detail to the docstring.

mo-jbeaver · 2026-07-27T10:48:29Z

+            candidate_name: Name of the secondary input.
+            fps: Forecast period values in seconds to extract.
+            cubes: CubeList containing all input cubes.
+            target_grid_cube: Target grid cube for optional regridding.


Is the regridding optional?

I've tried to clarify that the target_grid_cube variable can be optional and updated the type hints and docstrings to reflect this.

gavinevans added 3 commits July 23, 2026 21:28

Correct methodology for matching to clusters when the realizations in…

29e9901

… the secondary forecast source are fewer than the number of clusters.

The previous commit exposed a possible error where the number of real…

2d4e0c7

…izations in the secondary inputs changed over time, so a merge would fail. This has now been handled by curtailing any secondary input if there is a change in the number of realizations.

Refactoring to centralise calls across _process_partial_realization_i…

6fc8268

…nputs and _process_full_realization_inputs.

gavinevans assigned mo-jbeaver Jul 24, 2026

gavinevans mentioned this pull request Jul 27, 2026

Support deterministic forecast source when clustering gavinevans/improver#20

Open

2 tasks

mo-jbeaver requested changes Jul 27, 2026

View reviewed changes

mo-jbeaver assigned gavinevans and unassigned mo-jbeaver Jul 27, 2026

Edits following review.

eb067fb

gavinevans assigned mo-jbeaver and unassigned gavinevans Jul 27, 2026

		@@ -705,8 +717,31 @@ def _categorise_secondary_inputs(
		)
		continue # No valid forecast periods after filtering

		)


		def test_clusterandmatch_full_input_inconsistent_realization_counts_truncates():

		Updates replaced_realizations, cluster_sources, and
		secondary_input_realizations_to_clusters in-place. Called from the

Uh oh!

Conversation

gavinevans commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mo-jbeaver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gavinevans commented Jul 24, 2026 •

edited

Loading