Refactor sampling component from ObservationRecipe into SampleBuilder by ph-kev · Pull Request #334 · CliMA/ClimaCalibrate.jl

ph-kev · 2026-06-30T00:32:43Z

closes #328 - This PR refactors the sampling part from ObservationRecipe into SampleBuilder.

All relevant functions in ObservationRecipe takes in a ObservedSampleCollection which does all the checks of the OutputVars. This eliminates most of the checks from ObservationRecipe and move them to SampleBuilder.

The SampleBuilder module features a SampleCollection struct and an ObservedSampleCollection struct. A SampleCollection struct features a matrix of samples and its associated metadata and an ObservedSampleCollection struct also includes which sample will be the observation in the calibration. These two structs are separated to allow for future changes to how the observation is chosen without breaking changes.

For the future, we might be interested in including NaNs in the matrix of samples and letting ObservationRecipe.observation handle the NaNs. In addition, we might also want hcat and vcat to be define for ObservedSampleCollection to allow for more flexibility in constructing this object.

TODO

Add to NEWS.md
Update ObservationRecipe docs
Check tests

ph-kev · 2026-06-30T00:34:39Z

+ClimaCalibrateClimaAnalysisExt.SampleCollection
+ClimaCalibrate.SampleBuilder.generate_samples
+ClimaCalibrate.SampleBuilder.generate_samples_by_times
+ClimaCalibrate.SampleBuilder.num_samples
+ClimaCalibrate.SampleBuilder.reconstruct_col
+ClimaCalibrateClimaAnalysisExt.ObservedSampleCollection


I don't like the names SampleCollection and ObservedSampleCollection and I think there are probably better names for these two structs. I am against names that include the word "matrix" in them since the structs don't implement the AbstractArray interface.

How about StackedSamples or FlatStackedOutputVars. as a couple more options... for samples collection

codecov · 2026-06-30T04:16:48Z

Codecov Report

❌ Patch coverage is 99.55157% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.06%. Comparing base (939e2c3) to head (68cbef7).

Files with missing lines	Patch %	Lines
ext/sample_builder.jl	99.22%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   60.64%   61.06%   +0.42%     
==========================================
  Files          18       19       +1     
  Lines        1466     1482      +16     
==========================================
+ Hits          889      905      +16     
  Misses        577      577

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ph-kev · 2026-07-01T23:08:41Z

+You can exclude dimensions from these checks with the `ignore_dims` keyword
+argument. This is useful when the samples are meant to differ along a dimension.


This feature might be needed for the GEnsembleBuilder but it is not needed right now.

nefrathenrici

Thank you, covariance and observation are much cleaner now!

nefrathenrici · 2026-07-02T15:13:58Z

+    `ObservationRecipe.covariance` now take an `ObservedSampleCollection` (e.g
+    `observation(covar_estimator, osc)`) instead of
+    `(covar_estimator, vars, start_date, end_date)`. The `dims` keyword (flatten
+    order) is moved to `SampleBuilder.generate_samples`.


This needs to be updated

odunbar

Hi @ph-kev This is great.

I have a few comments - that I would be keen to discus.

Do we need a separate module for Samples builder?
I'm not sure if we actually need a separate object for ObservaionCollection, is this actually useful? Or should it just be another argument in observation(...)
I think the docs, and possibly the internal storage too, should separate between a "vector of (stacked-) vectors" and a matrix. As it seems we end up with needing lots of matrix return methods to interrogate the samples collection objects despite them fundamentally not being a matrix any more.

odunbar · 2026-07-11T00:26:48Z

+machine learning algorithms, observations are represented as a vector of numbers
+along with a covariance matrix describing the uncertainty of the observation.
+However, the diagnostics of climate models are typically in the form of gridded
+fields with named dimensions such as latitude, longitude, and time.


I think maybe it would be more helpful to say "On the other hand, diagnostics from ClimaAnalysis are stored as "OutputVars", which store them as e.g. ...(be more explicit)

odunbar · 2026-07-17T20:02:20Z

+However, the diagnostics of climate models are typically in the form of gridded
+fields with named dimensions such as latitude, longitude, and time.
+
+The `SampleBuilder` module bridges this gap. It flattens one or more


"...to bridge this gap, three operations need to be performed.

we flatten each OutputVar into a vector, and store information for the flattening/unflattening operation. This is implemented by SamplesBuilder.build_samples constructing a SampleBuilder.SampleCollection.

we construct the associated covariance matrix for the flattened samples. This is typically prescribed or estimated from the flattened samples. This is dispatched by defining <: ObservationRecipe.AbstractCovarianceEstimator types

we combine these in an EKP.Observation. Implemented by the ObservationRecipe.observation "

odunbar · 2026-07-17T20:09:26Z

+metadata and packages the result as a [`SampleCollection`](@ref
+ClimaCalibrateClimaAnalysisExt.SampleCollection). From that matrix you pick one
+column as the true observation to create an [`ObservedSampleCollection`](@ref
+ClimaCalibrateClimaAnalysisExt.ObservedSampleCollection). The


The ObservedSampleCollection feels a bit cumbersome, i can't see why it is necessary to have this as an actual object. What you choose to take as an observation is a property of the experiment, not a property of the samples. For example, would it not feel more natural for this to be passed in as a third argument to ObsRecipe.observation?

i'm also thinking about things like how would minibatching fit into your framework here?

odunbar · 2026-07-17T20:19:04Z

+
+```math
+\begin{pmatrix}
+\mathrm{OV}_{11} & \cdots & \mathrm{OV}_{1m} \\


I'd use V for the OutputVar not "OV" as in mathmode this looks like a product of two variables. also the "math" term for flatten is vec. e.g.

$$v_{ij} = \mathrm{vec}(V_{ij})$$

I am a bit hung up on the the matrix idea. Would it be correct in thinking that really this is basically just a guard to ensure that the user provides a sample of each variable? If so, when presenting the math and describing the structure we should use maybe the following notations:

$$ \begin{pmatrix} (\mathrm{V}^{(1)})_1 & \cdots & (\mathrm{V}^{(1)})_m\\ \vdots & \ddots & \vdots \\ (\mathrm{V}^{(n)})_1 & \cdots & (\mathrm{V}^{(n)})_m \end{pmatrix} \rightarrow \left(\begin{pmatrix} (\mathrm{v}^{(1)})_1 \\ \vdots \\ (\mathrm{v}^{(n)})_1 \end{pmatrix}, \dots, \begin{pmatrix}(\mathrm{v}^{(1)})_m \\ \vdots \\ (\mathrm{v}^{(n)})_m\end{pmatrix} \right) $$

This clearly indicates n variable types and m samples of these variables. Removing the ambiguity of the dimensions, and highlighting why we would have a "matrix" of OutputVars in the first place.

odunbar · 2026-07-17T21:03:54Z

+ClimaCalibrateClimaAnalysisExt.SampleCollection
+ClimaCalibrate.SampleBuilder.generate_samples
+ClimaCalibrate.SampleBuilder.generate_samples_by_times
+ClimaCalibrate.SampleBuilder.num_samples
+ClimaCalibrate.SampleBuilder.reconstruct_col
+ClimaCalibrateClimaAnalysisExt.ObservedSampleCollection


How about StackedSamples or FlatStackedOutputVars. as a couple more options... for samples collection

odunbar · 2026-07-17T21:06:07Z

+There are also getter functions such as [`get_obs`](@ref) for getting the
+observation for the calibration, [`get_obs_metadata`](@ref) for getting the
+metadata of the observation, [`get_samples`](@ref) for getting the matrix of
+samples, [`get_metadata`](@ref) for getting the matrix of metadata.


It feels like really when dealing with the SamplesCollection objects we want to only get information about a sample or set of samples, (e.g. get a sample, get the metadata for the stacked sample etc.). But as we store it internally as a matrix - it leads to the need to do column indexing and return of matrix values. Would it not be cleaner to store internally as a vector of (stacked-) vectors?

ph-kev commented Jun 30, 2026

View reviewed changes

Fix bug for DimValuesChecker

c1875c8

ph-kev force-pushed the kp/sampler branch 5 times, most recently from 2018b80 to 96dd683 Compare June 30, 2026 22:08

ph-kev added 3 commits June 30, 2026 15:09

Add SampleBuilder module

80eb533

Refactor ObservationRecipe

950bfcd

Remove ObservationRecipe.change_data_type

68cbef7

ph-kev force-pushed the kp/sampler branch from 96dd683 to 68cbef7 Compare June 30, 2026 22:09

ph-kev requested review from nefrathenrici and odunbar June 30, 2026 22:09

ph-kev commented Jul 1, 2026

View reviewed changes

nefrathenrici approved these changes Jul 2, 2026

View reviewed changes

ph-kev mentioned this pull request Jul 13, 2026

Add Var.are_compatible for FlatVar and Metadata CliMA/ClimaAnalysis.jl#409

Open

odunbar requested changes Jul 17, 2026

View reviewed changes

		You can exclude dimensions from these checks with the `ignore_dims` keyword
		argument. This is useful when the samples are meant to differ along a dimension.

Uh oh!

Conversation

ph-kev commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nefrathenrici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odunbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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!

3 participants

ph-kev commented Jun 30, 2026 •

edited

Loading

codecov Bot commented Jun 30, 2026 •

edited

Loading

odunbar Jul 17, 2026 •

edited

Loading

odunbar Jul 17, 2026 •

edited

Loading