Design mllam-verification package #2

mafdmi · 2025-02-04T12:25:55Z

This PR is not meant for being merged, but to facilitate the discussion of how we want to design the mllam-verification package. See the README.md for the initial thoughts on the design

mafdmi · 2025-02-04T13:15:41Z

README.md

See https://github.com/mafdmi/mllam-verification/blob/feature/design-package/README.md for proper rendering

mafdmi · 2025-02-04T13:16:19Z

README.md

+- Every plot function should have ax = None as input argument. If no axes is provided, a new figure with appropriate settings/features (e.g. coastlines for maps) should be created. Otherwise, the plot should be added to the provided axes on top of the existing plot with a zorder = 10*n, where n is an integer. One can also specify zorder as input argument to the plot function to place the plot at a specific place in the plot hierarchy.
+- Every plot function should have an include_persistence input argument, that defaults to True if it is possible to add persistence for the given plot type, and False if not. If include_persistence = True , but the plot doesn't support plotting the persistence an error should be raised.
+- Every plot function should take the metric to compute and plot as input argument.
+- The functions shall be callable from JupyterNotebook/Python script and the CLI.


Comment by @observingClouds imported from confluence

CLI sounds complicated, especially to defined variables, datasets etc.

Okay, so we should not have a CLI?

mafdmi · 2025-02-04T13:16:36Z

README.md

+- Every plot function should take the metric to compute and plot as input argument.
+- The functions shall be callable from JupyterNotebook/Python script and the CLI.
+- The top-level plot functions should be named according to the plot they produce. They should not directly contain the logic to actually compute the metrics, but instead call other comput functions to do that.
+- The package shall support specifying plot settings, output path for saving plots, etc. in a config file


Do we want this, or should people just adjust the axes/figure by themselves after calling the plot functions?

Comment by @observingClouds imported from confluence:

I think the package should stick to the minimum and does not provide any config parser. Just simple functions with input arguments and axes or values as return values, depending on the functions aim.

leifdenby · 2025-02-04T13:23:59Z

mllam_verification/plot.py

+    ds_metric = mlverif.calculate_{metric}(ds_reference, ds_prediction, variable, include_persistence=include_persistence)
+
+    if axes is None:
+        axes = mlverif.operations.plot_utils.get_axes(plot_type="timeseries")


I would just init a new figure directly here rather than putting that somewhere else, that would be more explicit

Suggested change

axes = mlverif.operations.plot_utils.get_axes(plot_type="timeseries")

fig, axes = plt.subplots()

I just thought, that this step would also include e.g. setting up projection, coastline etc. if needed, so it might be more than just a oneliner.

leifdenby · 2025-02-04T13:26:58Z

mllam_verification/plot.py

+    ds_metric[metric].plot.line(ax=axes, **xarray_plot_kwargs)
+
+    if include_persistence:
+        ds_metric["persistence"].plot.line(ax=axes, **xarray_plot_kwargs)


Maybe the metric calculation could add a new coordinate to the dataset called say data_source which could include names like [persistence, DINI_forecast, ...] (persistence would only be there if include_persistence==True, the other names could be set from dict keys if we pass in multiple prediction datasets), then plotting could be replaced with:

Suggested change

ds_metric[metric].plot.line(ax=axes, **xarray_plot_kwargs)

if include_persistence:

ds_metric["persistence"].plot.line(ax=axes, **xarray_plot_kwargs)

ds_metric[metric].plot.line(ax=axes, hue="data_source", **xarray_plot_kwargs)

I don't think we should need to select the metric variable here, either we compute the same metric for all variables and select a specific variable here, or only calculate the metric for a specific variable, in which case the return from the metric function could just be a data-array

Regarding my comment above, maybe we should rather than using a dict require that the user does concatenation along a data_source dimension if the ds_prediction argument is to contain multiple sources

I don't think we should need to select the metric variable here, either we compute the same metric for all variables and select a specific variable here, or only calculate the metric for a specific variable, in which case the return from the metric function could just be a data-array

Agree, the [metric] part is not needed. I think we should stick with only calculating one metric for one variable and let the metric function return a data-array then.

Regarding my comment above, maybe we should rather than using a dict require that the user does concatenation along a data_source dimension if the ds_prediction argument is to contain multiple sources

We could do that yet. Not sure which one is the better design.

…ic_hovmoller and plot_single_metric_gridded_map plots

mafdmi · 2025-02-12T09:13:52Z

mllam_verification/plot.py

+import mllam_verification.operations.statistics as mlverif_stats
+
+
+def plot_single_metric_timeseries(


I'm a bit in doubt how we want to include the possibility to use either grouped, elapsed, UTC or Multiple times in this and other plot functions. Should we have an input parameter like time_type: str = Literal["grouped"|"elapsed"|"UTC"|"multi"] and then have an extra optional groupby: Optional[str] = None argument? Or should we have four different input arguments groupby: Optional[str] = None, elapsed: Optional[bool] = False, utc: Optional[bool] = False and multi: Optional[bool] = False? Or do you have other ideas?

mafdmi · 2025-02-12T09:14:58Z

mllam_verification/plot.py

+    ds_metric: xr.Dataset = stats_operation(
+        ds_reference, ds_prediction, reduce_dims=["grid_index"]
+    )
+    if "start_time" in ds_metric.dims:


I'm in doubt if we want to make the user decide based on an input argument whether we should do the mean over start_times. For now, I assumed that we would do the mean if the dimension is present, since otherwise the plot doesn't work.

mafdmi · 2025-02-12T09:15:59Z

mllam_verification/plot.py

+
+    ds_metric[variable].plot.line(ax=axes, hue=hue, **xarray_plot_kwargs)
+
+    return axes


Should the user also be able to pass a figure object? Then I would return it here as well.

mafdmi · 2025-02-12T09:20:38Z

mllam_verification/plot.py

+    return axes
+
+
+def plot_single_metric_gridded_map(


I'm unsure if we want to support passing a stats_operation function to this plot also, since for now the operation I do is just the difference between ds_prediction and ds_reference. If we want to support this, we should probably just define a

def difference(ds_prediction: xr.Dataset, ds_reference: xr.Dataset) -> xr.Dataset: return ds_prediction - ds_reference

function in statistics.py, and then make the new input argument default to this: stats_operation: FunctionType = mlverif_stats.difference.

What do you think?

…istics functions work both on dataarrays and datasets. Added some tests for statistics.py file

mafdmi · 2025-02-12T09:59:32Z

tests/unit/conftest.py

This file is still under development together with tests, so don't use time to review it.

mafdmi · 2025-02-12T10:01:20Z

mllam_verification/operations/statistics.py

+            # Update the cell_methods with the operation applied
+            cell_methods.append(f"{groupby}: groupby")
+
+    if stats_op:


We could add cell_methods for the stats_op here as well, e.g. simply use the stats_op name as the cell_method. But since I thought, that that isn't necessarily cf compliant, I decided to keep it in the individual rmse/mae/mean/etc. functions.

mafdmi · 2025-02-12T10:05:06Z

I've now developed the first three plotting functions and related statistical functions according to the new API design (as much as I could). I think I would like to get some feedback on the implementation before I continue using more time on it. In particular, I've raised a bunch of questions in the plot.py and statistics.py modules, which I would like input on.
@matschreiner or @observingClouds would you have time to look at it?

mafdmi added 3 commits February 4, 2025 13:21

Add function designs from confluence

5bf4e8d

Added design ideas to README

0183528

Added plot examples

26d7712

mafdmi commented Feb 4, 2025

View reviewed changes

leifdenby reviewed Feb 4, 2025

View reviewed changes

mafdmi added 4 commits February 11, 2025 14:17

Added first plot methods for the new design of the package

d276db3

Refactored and fixed updating of cell_methods. Added plot_single_metr…

00ca854

…ic_hovmoller and plot_single_metric_gridded_map plots

Updated docstrings, moved function.

0713ffc

Removed obsolete metrics.py file

f11c06d

mafdmi commented Feb 12, 2025

View reviewed changes

Made plot functions work on dataarrays instead of datasets. Made stat…

1e6f28c

…istics functions work both on dataarrays and datasets. Added some tests for statistics.py file

mafdmi commented Feb 12, 2025

View reviewed changes

tests/unit/conftest.py

Copy link

Collaborator Author

mafdmi Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is still under development together with tests, so don't use time to review it.

mafdmi commented Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design mllam-verification package #2

Design mllam-verification package #2

mafdmi commented Feb 4, 2025 •

edited

Loading

mafdmi Feb 4, 2025

mafdmi Feb 4, 2025

mafdmi Feb 4, 2025

mafdmi Feb 4, 2025

mafdmi Feb 4, 2025

leifdenby Feb 4, 2025

mafdmi Feb 4, 2025

leifdenby Feb 4, 2025

leifdenby Feb 4, 2025

leifdenby Feb 4, 2025

mafdmi Feb 4, 2025

mafdmi Feb 4, 2025

mafdmi Feb 12, 2025

mafdmi Feb 12, 2025

mafdmi Feb 12, 2025

mafdmi Feb 12, 2025

mafdmi Feb 12, 2025

mafdmi Feb 12, 2025 •

edited

Loading

mafdmi commented Feb 12, 2025

	axes = mlverif.operations.plot_utils.get_axes(plot_type="timeseries")
	fig, axes = plt.subplots()

		import mllam_verification.operations.statistics as mlverif_stats


		def plot_single_metric_timeseries(


		ds_metric[variable].plot.line(ax=axes, hue=hue, **xarray_plot_kwargs)

		return axes

Design mllam-verification package #2

Are you sure you want to change the base?

Design mllam-verification package #2

Conversation

mafdmi commented Feb 4, 2025 • edited Loading

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

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

mafdmi Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

mafdmi commented Feb 12, 2025

mafdmi commented Feb 4, 2025 •

edited

Loading

mafdmi Feb 12, 2025 •

edited

Loading