feat/add-ensemble-metrics-crps-spread - #226
Conversation
|
Hi, could you check how this relates to #229 ? Seems like these is potentially a lot of overlap, and it would be great if we could focus community efforts on one of these PRs. |
|
Hi @joeloskarsson, thanks for pointing this out! I’ve reviewed #229 by @Panchadip-128 and there is definitely a large overlap on the CRPS implementation and test infrastructure. PR #229 is very comprehensive and introduces excellent ensemble mode configurations. To align with the community and avoid duplicated effort, I am more than happy to close this PR in favor of #229. @Panchadip-128 I noticed my branch also ports the spread_squared (ensemble variance) metric, which might be a useful complement to CRPS for evaluating probabilistic spread. Let me know if you’d like me to open a PR against your branch to add it, or if it would be better for me to wait and open a follow-up PR adding spread_squared once #229 is merged into main! |
|
I've been a bit reluctant to implement an ensemble CRPS metric before we have any model that can use it, but given that there seem to be so much interest in this I think we can start looking at it. Here are some quick scattered thoughts around CRPS metric:
Sharing a very general CRPS method that I have for another project (for inspiration): Should naturally be adapted to the neural-lam metric API. But this gives an idea for how to get the fair/unfair/almost-fair versions in an easy way. |
|
Hi @joeloskarsson and @shauryam2807, Thanks for the detailed notes and for sharing the reference CRPS estimator — very helpful. I’ve been looking into how this could integrate cleanly with the current metrics API and the forecast tensor shapes proposed in #335. A few thoughts that might help structure the implementation:
it may be useful for the metric to accept an explicit
I also sketched a small evaluation flow diagram based on #335 + CRPS integration to help ground the discussion — happy to refine or adjust if useful. If helpful, I can follow up with a minimal implementation aligned with this structure (e.g., base estimator + rank-based spread term) and keep it tightly scoped. |
|
one thing that feels important to lock down early here is that especially if there is any |
|
Good point - keeping crps_ens entry-wise until after mask_and_reduce_metric ensures consistency with deterministic metrics, so masking, latitude weighting (grid_weights), and variable reductions apply uniformly across both paths. That kind of subtle divergence is very hard to catch later when comparing deterministic vs ensemble evaluation. On S=1: since single-member CRPS is undefined as a proper scoring rule, an explicit assert S > 1 early in the function seems cleaner than silently short-circuiting - it also guarantees the reduction pipeline is never accidentally bypassed. Any future changes to reduction semantics (e.g., new masking strategies) then automatically apply to ensemble metrics without special-casing. Happy to help align the implementation with this if useful. |
|
Hi @joeloskarsson, @shauryam2807, @Panchadip-128, @kshirajahere — Entry-wise computation → mask_and_reduce_metric internally (same pattern as mse, nll, etc.), so grid_weights, boundary masking, and sum_vars apply uniformly across deterministic and ensemble eval paths |
|
@Sir-Sloth-The-Lazy can you explain why we need a separate PR for the ensemble spread? That is already added here, so #431 seems just like a duplicate of this now. Let me know if I am missing something. |
| depending on reduction arguments. | ||
| """ | ||
| num_ens = pred.shape[ens_dim] # Number of ensemble members | ||
| if num_ens == 1: |
There was a problem hiding this comment.
the N=1 branch has two issues when delegating to mae:
pred_stdis hardcoded asNone, which causes aTypeError
(ones_like(): argument must be Tensor, not NoneType) if the caller
passes apred_stdtensorsum_varsis silently dropped, sosum_vars=Falseis ignored
and output shape diverges from theN>1path
verified both locally. fix:
if num_ens == 1:
return mae(
pred.squeeze(ens_dim),
target,
pred_std.squeeze(ens_dim) if pred_std is not None
else torch.ones_like(target),
mask=mask,
average_grid=average_grid,
sum_vars=sum_vars,
)also no N=1 test case exists which is likely why this slipped
through ...best to add one to test_metrics.py
There was a problem hiding this comment.
also wrote a test that catches both issues — crashes on current
code, passes with the fix:
def test_crps_ens_single_member():
# N=1 reduces to MAE — verify sum_vars and pred_std are correctly
# passed through. Previously both were silently dropped causing a
# TypeError crash and incorrect output shapes.
pred = torch.tensor([[[[1.0, 2.0, 3.0]]]]) # shape (1, 1, 1, 3)
target = torch.tensor([[[2.0, 2.0, 2.0]]]) # shape (1, 1, 3)
pred_std = torch.ones_like(pred)
result_summed = crps_ens(
pred, target, pred_std,
sum_vars=True, ens_dim=1
)
result_not_summed = crps_ens(
pred, target, pred_std,
sum_vars=False, ens_dim=1
)
assert result_not_summed.shape[-1] == 3, \
"sum_vars=False should preserve feature dimension in N=1 path"
assert result_summed.shape != result_not_summed.shape, \
"sum_vars flag ignored in N=1 path"There was a problem hiding this comment.
Why would we need to pass any pred_std to mae, that is not used there? We could always pass it as None, no matter what was input to crps_ens, as the crps should not depend on predicted std.
|
@joeloskarsson To clarify my point of view, #226's spread_squared is bundled with crps_ens in a branch that currently has review issues (duplicate DEFINED_METRICS), and can't merge until the CRPS design questions are resolved (estimator variants, rank-based implementation, internal batching). |
|
Hello @joeloskarsson — Thank you for sharing the reference CRPS implementation. I'll refactor my implementation to follow your design: rank-based O(M log M) approach with biased/unbiased/almost-fair estimator variants, aligned with the tensor shapes from #335. @GiGiKoneti — Great catches on the N=1 path bugs. I'll fix both the pred_std=None crash and the dropped sum_vars flag, and add the test case you suggested. I'll push the updated implementation shortly. |
|
@GiGiKoneti One small addition to the test: it would be worth also verifying that the N=1 path and the N>1 path produce consistent results when S=1 reduces to a single-member forecast - specifically that mask is still applied correctly in both cases, since that's the subtle divergence that's hardest to catch. |
|
verified locally on current PR #226 code (fix not yet pushed) —
because
once the fix lands: if num_ens == 1:
return mae(
pred.squeeze(ens_dim),
target,
pred_std.squeeze(ens_dim) if pred_std is not None
else torch.ones_like(target),
mask=mask,
average_grid=average_grid,
sum_vars=sum_vars,
)@Panchadip-128's mask consistency test should pass correctly |
GiGiKoneti
left a comment
There was a problem hiding this comment.
reviewed current branch — fixes not yet pushed.
comments left inline on the N=1 path. will re-review
once the update lands.
joeloskarsson
left a comment
There was a problem hiding this comment.
Read over and added some comments. Some of these are more discussion points, that I think we need to make some decisions on the best way to handle.
|
|
||
| # ------------------------------------------------------------------ | ||
| # Spread term factor depends on estimator choice | ||
| # ------------------------------------------------------------------ |
There was a problem hiding this comment.
These comments with all the dashed lines seem unnecessarily large
|
Hello
New tests added:
All 11/11 tests pass ✅ |
| elif estimator == "unbiased": | ||
| diff_factor = 1 / (num_ens - 1) | ||
| elif estimator == "almost-fair": | ||
| if afc_alpha is None: |
There was a problem hiding this comment.
Naveau / AIFS formulation assumes α ∈ (0, 1) — passing afc_alpha=0 collapses to the unbiased estimator silently, and values outside (0, 1) produce mathematically invalid diff_factor. A simple guard would prevent silent misuse:
if not (0 < afc_alpha < 1):
raise ValueError(
f"afc_alpha must be in (0, 1) for almost-fair CRPS, got {afc_alpha}"
)There was a problem hiding this comment.
Good idea, yes checking that afc_alpha is in [0,1] would be good.
|
@shauryam2807 Thanks for incorporating the fixes! I've pulled the latest changes ( |
|
@shauryam2807 are you willing to work on this PR ? I would be really happy to help ! :) Great work here ! Just pinging @joeloskarsson if you have time to review this 😁 |
joeloskarsson
left a comment
There was a problem hiding this comment.
My previous comments are resolved, but this needs another look over as there are some conflicts to resolve and unrelated changes in the PR.
@shauryam2807 as @Sir-Sloth-The-Lazy mention above, he is happy to help as this relates to his overarching work of enabling probabilistic forecasting in the repo. Do let us know if you want to keep doing the work on this, get some help from @Sir-Sloth-The-Lazy, or hand over this fully to him.
| sum_vars=True, | ||
| ens_dim=1, | ||
| estimator="unbiased", | ||
| afc_alpha=None, |
There was a problem hiding this comment.
It would be good to add also specific biased, unbiased and almost-fair (with exactly afc_alpha=0.95) versions of crps_ens, so that they could be used in the same interface as other metrics, without having to deal with the afc_alpha argument in some special way. Suggestion to use functools.partial.
| mask=None, | ||
| average_grid=True, | ||
| sum_vars=True, | ||
| ens_dim=1, |
There was a problem hiding this comment.
Do we need to allow for different ens_dim here? This is something we need to enforce throughout the codebase, so might be better to just specify clearly that we assume dim 1 to be the ens dim in all inputs.
There was a problem hiding this comment.
I agree, Should we expose ens_dim as a configurable argument at all? Per the shape contract in #335 the ensemble axis is fixed at dim 1 ((B, S, T, N, F)), and we want that enforced consistently across the codebase rather than per-call. Suggest dropping the parameter and instead documenting/asserting that S is assumed at dim 1, that keeps the deterministic and ensemble eval paths from quietly diverging if a caller passes a different value. If we do keep it, it should at least be validated against the expected layout.
|
Hello @joeloskarsson and @Sir-Sloth-The-Lazy, Thank you for reviewing my PR. I will address all the conflicts and suggestions mentioned in the review. I am currently occupied with my end-semester examinations and my responsibilities as a mentee in another project, so there may be some delay in my responses. However, I remain committed to this contribution and will ensure that the PR is properly maintained and updated. I am also fully capable of maintaining this PR and will continue to actively work on it until all review comments and requested changes have been resolved. Thank you for your understanding |
|
Closing this in favour of #649, which consolidates the probabilistic metrics + ensemble plumbing track for v0.8.0. The sample-based CRPS and |
|
oh yeah this one was a bit too eagerly closed for consolidation, sorry @shauryam2807 and thanks for your work here! |
|
Hello @joeloskarsson @sadamov @Sir-Sloth-The-Lazy Yes I am exicted to work on this as I said you that I will mange it and work on it Thank you for your time and believe |
|
Hi @shauryam2807, I am trying to coordinate and push this through as quickly as possible. This PR is a high priority to us, since it directly relates to the work @Sir-Sloth-The-Lazy is doing with probabilistic models as part of his GSoC project. Could you let us know your estimate for when you have time to look at this? We would really need to make some progress on this within a timeframe of ~ a week. If this does not seem feasible to you we will have to let @Sir-Sloth-The-Lazy take over the main work on this. I encourage you to instead collaborate on this PR, and let @Sir-Sloth-The-Lazy help with finalizing this work. |
|
"Hi @joeloskarsson, I am available and will prioritize this. I will resolve the conflicts and have this ready for you to review within 2-3 days. |
Port crps_ens (biased/unbiased/almost-fair estimators) and spread_squared from prob_model_lam. Uses rank-based O(M log M) implementation. Includes 11 test cases. Refs mllam#62 Co-authored-by: Gemini <gemini@google.com>
e4f1b07 to
3be0905
Compare
|
Hello @joeloskarsson, I have rebased the branch on latest main and cleaned up all unrelated file changes. The PR now only touches metrics.py and test_metrics.py. All previous review comments have been addressed. Ready for your review. |
Sir-Sloth-The-Lazy
left a comment
There was a problem hiding this comment.
I went through the prob_model_lam branch this is ported from to confirm coverage. Good news: the function set matches (crps_ens + spread_squared), and this PR is actually ahead of the source by adding the biased / almost-fair estimators on top of the unbiased one. 👍
One functional gap though: the source's large-ensemble (M >= 10) memory-batched branch wasn't carried over, since it's the OOM case flagged earlier and the ensemble eval path will hit it. Everything else the ensemble models depend on (spread-skill ratio, ens_mse) lives in the model code, not here, so it's out of scope for this PR, spread_squared(..., sum_vars=False) already returns the shape that path needs.
| Returns: | ||
| metric_val: shape depends on reduction arguments. | ||
| """ | ||
| import warnings |
There was a problem hiding this comment.
This is an odd place to import 😅
| return mae( | ||
| pred.squeeze(ens_dim), | ||
| target, | ||
| pred_std.squeeze(ens_dim) | ||
| if pred_std is not None | ||
| else torch.ones_like(target), | ||
| mask=mask, | ||
| average_grid=average_grid, | ||
| sum_vars=sum_vars, | ||
| ) |
There was a problem hiding this comment.
If a caller passes pred_std of shape (F,) (a valid form elsewhere in the metrics API), .squeeze(ens_dim) raises IndexError: Dimension out of range. Since crps_ens doesn't use pred_std at all, we can follow Joel's earlier suggestion and just pass None unconditionally:
| return mae( | |
| pred.squeeze(ens_dim), | |
| target, | |
| pred_std.squeeze(ens_dim) | |
| if pred_std is not None | |
| else torch.ones_like(target), | |
| mask=mask, | |
| average_grid=average_grid, | |
| sum_vars=sum_vars, | |
| ) | |
| return mae( | |
| pred.squeeze(ens_dim), | |
| target, | |
| torch.ones_like(target), | |
| mask=mask, | |
| average_grid=average_grid, | |
| sum_vars=sum_vars, | |
| ) |
| mask=None, | ||
| average_grid=True, | ||
| sum_vars=True, | ||
| ens_dim=1, |
There was a problem hiding this comment.
I agree, Should we expose ens_dim as a configurable argument at all? Per the shape contract in #335 the ensemble axis is fixed at dim 1 ((B, S, T, N, F)), and we want that enforced consistently across the codebase rather than per-call. Suggest dropping the parameter and instead documenting/asserting that S is assumed at dim 1, that keeps the deterministic and ensemble eval paths from quietly diverging if a caller passes a different value. If we do keep it, it should at least be validated against the expected layout.
|
Hi @shauryam2807 , if you could look over the unresolved comments from Joel's review and solve them, I would be able to ping him for another review and we can get this PR merged sooner. Thank you for your hardwork 🤩 |
|
Hi @joeloskarsson, regarding the large-ensemble OOM gap that @Sir-Sloth-The-Lazy flagged — I've been looking into a few approaches to handle memory efficiently for
My thoughts: However, before I implement this, I wanted to get your input on a few things:
Happy to implement whichever direction you think fits the project best! |
|
Hi, weighing in on the OOM aspect. In general this is not super crucial to tackle optimally here, and something that can be optimized further later. I also prefer approach 1 here, and configurable chunk size as keyword arguments, defaulting to no such chunking. In particular, I don't think this should be based on the number of ens members as in the I would strongly disagree with adding in I don't understand the difference between the PWM approach (suggestion 3) and the current one, but this does not seem like it has potential to save that much. I thought we are following the PWM formulation as given in Zamo and Naveau. Doing in-place sorting is risky, and better to be avoided imo. |
|
Hello To confirm, here's my plan based on your feedback:
I'll push the chunking update shortly. Thanks again for this |
|
I would chunk over some other dim than the grid if possible, since it seems tricky to choose a reasonable chunk size for that. Or make the choice of dim also an option. |
|
@shauryam2807, I hope you can get this work done a little faster, sorry for prying but this work is really important for visualisation PR #612 |
|
Hy @Sir-Sloth-The-Lazy Yes, I know it's been delayed due to a lot of other work I currently have on my plate. I do have a plan for this and will get it done as soon as possible! |
|
@shauryam2807 did you have sometime complete this work ? It would be really great if you did 😃 I am always here to help you have too much on your plate ! Hope you are doing well. |

Describe your changes
This PR ports the sample-based CRPS (
crps_ens) and ensemble variance (spread_squared) evaluation metrics from theprob_model_lambranch tomain.These metrics are essential for evaluating the probabilistic and ensemble models being developed (as tracked in Issue #62). I have also added a new test file
tests/test_metrics.pywith unit tests verifying the mathematical correctness and unbiased estimation of both metrics against expected values.Dependencies: None
Issue Link
Addresses parts of #62
Type of change
Checklist before requesting a review