Calibration superfluous edges #2033

SamGriffithsMO · 2024-09-26T09:02:54Z

Addresses https://github.com/MetOffice/improver_suite/issues/1750

Description

Updates gridded calibration CLI to align itself with spot calibration, e.g. The CLI now takes a directory and it is responsible for identifying which files are required.

This is additionally driven by a desire to remove >day long offsets in the suite configurations, as these require handling before producing a Cylc graph

I appreciate this is still in draft so likely you are doing a minimal change for testing. However, if/when you come closer to pulling this out of draft, we would want to ensure feature parity between the CLI and the plugin, whetehr that means changing EstimateCoefficientsForEnsembleCalibration UI and behaviour to call get_cube_from_directory, or otherwise defining a metaplugin to do achieve this.

I had thought about this issue, but hadn't considered a metaplugin, which I think might be the way to go. Thanks :)

cpelley · 2024-09-26T09:21:08Z

ping @gavinevans - you might be interested in this.

cpelley · 2024-09-26T09:21:46Z

Thanks @SamGriffithsMO, this looks very promising 👍

improver/calibration/__init__.py

SamGriffithsMO · 2024-09-27T15:46:05Z

Still wip

I am still looking for a performant method to extract the validity time from a netcdf, instead of from the filename
I need to write tests

cpelley · 2024-09-30T08:09:29Z

I am still looking for a performant method to extract the validity time from a netcdf, instead of from the filename

100% agree in using metadata rather information encoded in the filepath if possible.
However. Better metadata within the data from improver to increase traceability is something I have been wanting to address the entire time I have been here (on the 'todo' so-to-speak). If the metadata available right now isn't sufficient or deriving the validity time becomes performance inhibitive with the number of files involved, it might still be worth considering the filepath as a short term solution. An excuse for us to look at enforcing traceability through improved metadata within improver.

SamGriffithsMO · 2024-10-08T14:43:34Z

improver_tests/acceptance/SHA256SUMS

+0bd96af6cb5c6caa045e397589dd0ce3b498af837d989fe73326f5e9459c6054  ./construct-reliability-tables/basic/forecast/forecast_0.nc
+fbc14286b4ce41e2e60df0870ae4911c1b00a38ec96912f43c6187fcaf7d02f6  ./construct-reliability-tables/basic/forecast/forecast_1.nc
 0d0edf9751a2019db952907700b02499ec9f1c360db4591a8012ca247a841c73  ./construct-reliability-tables/basic/kgo_aggregated.nc
 902e5cb9d3dc5d2b78bb99aff8370f9815adf5064b2caeb7abed73a56a897a43  ./construct-reliability-tables/basic/kgo_single_value_bins.nc
 72d4fd0655d1b7a2bc11d85741ec944f195c59813ae629e6858116c4e09eccb0  ./construct-reliability-tables/basic/kgo_without_single_value_bins.nc
-8ed50464c34b8673d98d1256d1c11b9eeea911dc79f7f75d425a590bf8697301  ./construct-reliability-tables/basic/truth_0.nc
-3999adb3749052d9efdfab863427a20a1fabbca06ff430c6c9cf5f89d1ea4d60  ./construct-reliability-tables/basic/truth_1.nc
+8ed50464c34b8673d98d1256d1c11b9eeea911dc79f7f75d425a590bf8697301  ./construct-reliability-tables/basic/truth/truth_0.nc
+3999adb3749052d9efdfab863427a20a1fabbca06ff430c6c9cf5f89d1ea4d60  ./construct-reliability-tables/basic/truth/truth_1.nc


I've found best practices around the acceptances tests to be fairly obscure. This change is required to match the updated expected inputs for the cli, but it is unclear to me how we would implement this change for all users? And how much of a pain it would be?

If this is too much of a pain, I could make some changes to get_cube_from_directory

cpelley · 2024-10-10T13:06:59Z

improver/calibration/__init__.py

+
+    if len(cubes) < 2:
+        if verbose:
+            print(f"Not enough files found in {directory}")


Perhaps it would be good to include the number of cubes that was found in this message (i.e. 1 or 0)
I don't recall improver dev. guidelines around use of warnings etc. (you know?)

cpelley · 2024-10-10T13:09:20Z

improver/calibration/ensemble_calibration.py

@@ -1366,6 +1367,61 @@ def process(
        return coefficients_cubelist


+class MetaEstimateCoefficientsForEnsembleCalibration(BasePlugin):


This is a beast of a name 😋
How about this:

Suggested change

class MetaEstimateCoefficientsForEnsembleCalibration(BasePlugin):

class MetaEnsembleCoeffEstimator(BasePlugin):

cpelley · 2024-10-10T13:16:32Z

@SamGriffithsMO, have you given thought as to how to incorporate this change? (implications for BoM, USAF etc.)
An option to consider might be to create two new CLI's and making no functional change to the existing two CLI's. What you think?

cpelley · 2024-10-10T13:21:35Z

improver/calibration/ensemble_calibration.py

+        self.distribution = distribution
+        self.truth_attribute = truth_attribute
+        self.cycle_point = cycle_point
+        self.max_days_offset = max_days_offset
+        self.point_by_point = point_by_point
+        self.use_default_initial_guess = use_default_initial_guess
+        self.units = units
+        self.predictor = predictor
+        self.tolerance = tolerance
+        self.max_iterations = max_iterations


it's good form to suffix these with an underscore to indicate 'private' (considered private by convention, not in the same sense of private in c/c++ of course).
Basically, all data should be hidden within the class. If we really want to expose this data to users, then they should be exposed via properties (setters & getters). This stops users from easily tweaking values they aren't intended to be tweaking.

cpelley · 2024-10-10T13:22:15Z

improver/calibration/reliability_calibration.py

+        self.truth_attribute = truth_attribute
+        self.n_probability_bins = n_probability_bins
+        self.single_value_lower_limit = single_value_lower_limit
+        self.single_value_upper_limit = single_value_upper_limit
+        self.cycle_point = cycle_point
+        self.max_days_offset = max_days_offset
+        self.aggregate_coordinates = aggregate_coordinates


as per above

cpelley · 2024-10-10T13:22:41Z

improver/calibration/reliability_calibration.py

+
+        if forecast and truth:
+            plugin = ConstructReliabilityCalibrationTables(
+                # truth_attribute=self.truth_attribute,


what's this commended arg. here for?

cpelley · 2024-10-10T13:31:37Z

improver/cli/construct_reliability_tables.py

+    return MetaConstructReliabilityCalibrationTables(
+        truth_attribute=truth_attribute,
        n_probability_bins=n_probability_bins,
        single_value_lower_limit=single_value_lower_limit,
        single_value_upper_limit=single_value_upper_limit,
-    )(forecast, truth, aggregate_coordinates)
+        aggregate_coordinates=aggregate_coordinates,
+        cycle_point=cycle_point,
+        max_days_offset=max_days_offset,
+    )(forecast_directory, truth_directory)


Hmmmm, there is a lot to unpack here in terms of the implications going forwards.
Our future vision actually separates loading, polling, file identity from the IMPROVER plugins.
In this vision, I the existing IMPROVER plugins wouldn't be modified to identify the relevant files. Instead a plugin would specifically handle this and feed into the existing IMPROVER plugins.

We should chat offline.

cpelley · 2024-10-10T14:13:47Z

improver/cli/estimate_emos_coefficients.py

+    forecast_directory: cli.inputpath,
+    truth_directory: cli.inputpath,
+    land_sea_mask: cli.inputcube = None,


What about:

Suggested change

forecast_directory: cli.inputpath,

truth_directory: cli.inputpath,

land_sea_mask: cli.inputcube = None,

forecast_cubes: cli.calib_input_dir,

truth_cubes: cli.calib_input_dir,

land_sea_mask: cli.inputcube = None,

cpelley · 2024-10-10T14:14:08Z

improver/cli/estimate_emos_coefficients.py

        predictor=predictor,
        tolerance=tolerance,
        max_iterations=max_iterations,
    )
-    return plugin(forecast, truth, landsea_mask=land_sea_mask)
+    return plugin(forecast_directory, truth_directory, land_sea_mask=land_sea_mask)


What about:

Suggested change

return plugin(forecast_directory, truth_directory, land_sea_mask=land_sea_mask)

return plugin(**cubes)

cpelley · 2024-10-10T14:17:58Z

improver/cli/estimate_emos_coefficients.py

+    forecast_directory: cli.inputpath,
+    truth_directory: cli.inputpath,
+    land_sea_mask: cli.inputcube = None,


or actually (enabling continued CLI behaviour)

Suggested change

forecast_directory: cli.inputpath,

truth_directory: cli.inputpath,

land_sea_mask: cli.inputcube = None,

*cubes: Union[cli.calib_input_dir, cli.inputcube],

Caveat to this. I'm not a fan of the use of the clize typing mechanism within IMPROVER. However, this might facilitate maintaining existing CLI functionality and behaviour while also facilitating separating 'load' from the plugin itself (useful on our journey towards our long-term goals).

SamGriffithsMO · 2024-10-10T14:23:32Z

Putting in to draft to address what Carwyn and I discussed offline. Move the loading to clize (more inline with other plugins), and therefore allow the plugin to run whether given specific cubes or the directories as planed here

gavinevans

Thanks @SamGriffithsMO 👍

I'll written some comments below. I haven't totally finished but I'll post this now, as this PR has been pulled back into draft.

gavinevans · 2024-10-10T07:27:06Z

improver/calibration/__init__.py

+def get_cube_from_directory(
+    directory,
+    cycle_point=None,
+    max_days_offset=None,
+    date_format="%Y%m%dT%H%MZ",
+    verbose=False,
+):


Please could you add type hints for this function?

def get_cube_from_directory( directory: pathlib.Path, cycle_point: Optional[str], max_days_offset: Optional[int], date_format: str = "%Y%m%dT%H%MZ", verbose: bool = False, ) -> Cube:

gavinevans · 2024-10-10T07:29:31Z

improver/calibration/__init__.py

+    loads and merges all netCDF files in a directory
+
+    To switch on the max offset filter, both cycle_point and max_days_offset
+    need to be provided
+
+    Args:
+        directory (pathlib.Path):
+            The path to the directory.
+        cycle_point (str):
+            The cycle point of the forecast, used to filter files
+        max_days_offset (int):
+            Maximum number of days before cycle_point to consider files,
+            Defined as a postive int that is subtracted from the cycle_point
+        date_format (str):
+            format of the cyclepoint and datetime in the filename, used by
+            datetime.strptime
+        verbose (bool):
+            switch on verbose output


Could you remove the types from this docstring e.g. pathlib.Path as that should be added to the type hint, rather than here? Could you also capitalise the start of sentences and add full stops for readability?

gavinevans · 2024-10-10T07:29:57Z

improver/calibration/__init__.py

+            Maximum number of days before cycle_point to consider files,
+            Defined as a postive int that is subtracted from the cycle_point


Suggested change

Maximum number of days before cycle_point to consider files,

Defined as a postive int that is subtracted from the cycle_point

Maximum number of days before cycle_point to consider files.

Defined as a positive int that is subtracted from the cycle_point

gavinevans · 2024-10-10T07:31:24Z

improver/calibration/__init__.py

+            switch on verbose output
+
+    Returns:
+        Cube


Could you add something more descriptive here? E.g. Cube created by merging all netCDF files within the directory.

gavinevans · 2024-10-10T07:34:51Z

improver/calibration/ensemble_calibration.py

+    def __init__(
+        self,
+        distribution,
+        truth_attribute,
+        cycle_point=None,
+        max_days_offset=None,
+        point_by_point=False,
+        use_default_initial_guess=False,
+        units=None,
+        predictor="mean",
+        tolerance: float = 0.02,
+        max_iterations: int = 1000,
+    ):


Please could you add type hints here? These would be similar as already exist for the EstimateCoefficientsForEnsembleCalibration plugin.

gavinevans · 2024-10-10T11:25:00Z

improver/cli/estimate_emos_coefficients.py

        distribution,
+        truth_attribute,


This truth_attribute variable is actually no longer required in this CLI, as the forecasts and truths are being provided by separated directories, rather than this repository needing to separate the forecasts and truths, so I think the usage of truth_attribute in this CLI can be removed.

gavinevans · 2024-10-10T11:25:33Z

improver/calibration/ensemble_calibration.py

+    def __init__(
+        self,
+        distribution,
+        truth_attribute,


truth_attribute isn't used, so this can be removed.

gavinevans · 2024-10-10T14:10:41Z

improver/calibration/reliability_calibration.py

+        aggregate_coordinates: list = None,
+        cycle_point: Optional[str] = None,
+        max_days_offset: Optional[int] = None,
+    ):


Could you add docstrings for the __init__ and process methods?

gavinevans · 2024-10-10T14:11:13Z

improver/calibration/reliability_calibration.py

+
+        if forecast and truth:
+            plugin = ConstructReliabilityCalibrationTables(
+                # truth_attribute=self.truth_attribute,


I think the truth_attribute should be removed, as it's no longer used.

gavinevans · 2024-10-10T14:13:07Z

improver/cli/construct_reliability_tables.py

@@ -11,12 +11,16 @@
 @cli.clizefy
 @cli.with_output
 def process(
-    *cubes: cli.inputcube,
-    truth_attribute,
+    forecast_directory: cli.inputpath,


As noted by @cpelley, these changes would notably impact other users that use this CLI currently. We would need to communicate this.

SamGriffithsMO added 2 commits September 25, 2024 16:28

input directories for estimate emos

c3ee496

correct import

114776c

SamGriffithsMO added Type:TechnicalInfrastructure green_team labels Sep 26, 2024

SamGriffithsMO self-assigned this Sep 26, 2024

cpelley reviewed Sep 26, 2024

View reviewed changes

cpelley requested a review from gavinevans September 26, 2024 09:20

SamGriffithsMO added 3 commits September 27, 2024 08:56

meta estimate emos

b193539

reliabilty calibration

8187177

construct reliability tables accepts directories

66978c0

cpelley reviewed Sep 27, 2024

View reviewed changes

improver/calibration/__init__.py Outdated Show resolved Hide resolved

cpelley reviewed Sep 27, 2024

View reviewed changes

improver/calibration/__init__.py Outdated Show resolved Hide resolved

cpelley reviewed Sep 27, 2024

View reviewed changes

improver/calibration/__init__.py Outdated Show resolved Hide resolved

return no result if there is nothing to process

99a9c8b

SamGriffithsMO added 5 commits October 8, 2024 11:17

filter cubes by metadata

0d3019c

update acceptance tests

9cb95f7

reliability table acceptance tests - moved data

31a840d

tests for get_cube_from_directory

a599460

doc building

098fa58

SamGriffithsMO commented Oct 8, 2024

View reviewed changes

sort whitespace

95865ce

SamGriffithsMO marked this pull request as ready for review October 8, 2024 14:49

SamGriffithsMO requested a review from cpelley October 8, 2024 14:50

SamGriffithsMO mentioned this pull request Oct 10, 2024

Allow calibration CLI to retrieve nc from directories metoppv/improver_test_data#57

Draft

cpelley reviewed Oct 10, 2024

View reviewed changes

SamGriffithsMO marked this pull request as draft October 10, 2024 14:23

gavinevans reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calibration superfluous edges #2033

Calibration superfluous edges #2033

SamGriffithsMO commented Sep 26, 2024 •

edited

Loading

cpelley Sep 26, 2024

SamGriffithsMO Sep 26, 2024

cpelley commented Sep 26, 2024

cpelley commented Sep 26, 2024

SamGriffithsMO commented Sep 27, 2024

cpelley commented Sep 30, 2024 •

edited

Loading

SamGriffithsMO Oct 8, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024

cpelley commented Oct 10, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024

cpelley Oct 10, 2024 •

edited

Loading

cpelley Oct 10, 2024 •

edited

Loading

SamGriffithsMO commented Oct 10, 2024

gavinevans left a comment

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

gavinevans Oct 10, 2024

		forecast = get_cube_from_directory(forecast_directory, cycle_point=cycle_point, max_days_offset=max_days_offset)
		truth = get_cube_from_directory(truth_directory, cycle_point=cycle_point, max_days_offset=max_days_offset)

		@@ -1366,6 +1367,61 @@ def process(
		return coefficients_cubelist


		class MetaEstimateCoefficientsForEnsembleCalibration(BasePlugin):

	class MetaEstimateCoefficientsForEnsembleCalibration(BasePlugin):
	class MetaEnsembleCoeffEstimator(BasePlugin):

	return plugin(forecast_directory, truth_directory, land_sea_mask=land_sea_mask)
	return plugin(**cubes)

		Maximum number of days before cycle_point to consider files,
		Defined as a postive int that is subtracted from the cycle_point

Calibration superfluous edges #2033

Are you sure you want to change the base?

Calibration superfluous edges #2033

Conversation

SamGriffithsMO commented Sep 26, 2024 • edited Loading

Description

Related

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpelley commented Sep 26, 2024

cpelley commented Sep 26, 2024

SamGriffithsMO commented Sep 27, 2024

cpelley commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpelley commented Oct 10, 2024

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

cpelley Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

cpelley Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

SamGriffithsMO commented Oct 10, 2024

gavinevans 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

SamGriffithsMO commented Sep 26, 2024 •

edited

Loading

cpelley commented Sep 30, 2024 •

edited

Loading

cpelley Oct 10, 2024 •

edited

Loading

cpelley Oct 10, 2024 •

edited

Loading