Fix: timestep loop calculation in error map plotting - #582
Closed
Debadri-das wants to merge 1 commit into
Closed
Conversation
Changes prediction horizon loop to use len(time_slice) instead of enumerating over zipped predictions and targets, preventing indexing errors on ensemble dimensions. Co-authored-by: GitHub Copilot
Contributor
|
@Debadri-das i m closely working on this issue and have already created a PR on ur fork for it, just waiting on a merger of ur PR :) |
Author
|
@kshirajahere I’ve merged Debadri-das#1 on my fork, so the stacked change is unblocked now. Feel free to proceed with the next upstream step. You can suggest fixes in this PR too. Thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
This PR fixes the example-plotting semantics in
ARModel.plot_examples()for ensemble / probabilistic predictions.Summary of the problem
The current plotting path in
ARModel.plot_examples()was written for deterministic predictions with per-example shape:(pred_steps, num_grid_nodes, d_f)That assumption is embedded in how the method:
xarray.DataArraytime=t_i - 1during plottingvis.plot_prediction()After the ensemble export work in #521, prediction tensors can now carry an additional ensemble dimension, e.g.:
(B, S, T, N, F)At that point, the example-plotting path no longer has well-defined semantics. The current two-panel plotting interface (
Ground TruthvsPrediction) expects a single deterministic prediction field at each lead time, but an ensemble prediction carries multiple members for the same time step.In other words, the problem is not just that the tensor is “larger”; the problem is that the plotting contract is now ambiguous:
plot_examples()plot one member?Without resolving that ambiguity, the plotting path forwards data that still contains ensemble structure into code that expects a scalar spatial field, leading to misaligned indexing and eventual failure in plotting.
What this PR changes
This PR defines the semantics of the existing two-panel example plotting path for ensemble predictions by collapsing ensemble forecasts to their ensemble mean before plotting.
Concretely, the fix does the following inside
ARModel.plot_examples():(B, S, T, N, F)to(B, T, N, F)using the ensemble meanDataArrayconversion andvis.plot_prediction()interface unchangedWhy ensemble mean is the right default here
The current example-plotting utility is a simple two-panel qualitative visualization:
Ground TruthPredictionThat interface is built around comparing one deterministic forecast field against one target field at each time step.
Using the ensemble mean is the most conservative and compatible way to preserve that interface for probabilistic predictions because:
Scope of this PR
This PR is intentionally narrow.
It fixes only the semantic mismatch in
ARModel.plot_examples()caused by ensemble predictions entering a deterministic plotting path.It does not attempt to:
vis.plot_prediction()This means the PR is narrower than:
This PR is specifically about making the existing example plotting path meaningful and stable when predictions carry an ensemble dimension.
Motivation and context
Once 4D/5D ensemble support is introduced into the prediction/export pipeline, evaluation-time plotting becomes a natural follow-up integration point.
plot_examples()is one of the first places where deterministic assumptions surface clearly, because its plotting contract is qualitative and tightly coupled to “one field per lead time.” If that contract is not updated, the code can construct ensemble-aware tensors successfully but still fail during example visualization.By collapsing to ensemble mean inside
plot_examples(), this PR keeps the current plotting workflow operational while making the chosen semantics explicit, minimal, and easy to test.Test coverage
This PR includes a focused regression test covering the intended semantics:
Dependencies
No new dependencies are required.
This change uses only the existing project stack:
torchnumpyxarrayIssue Link
addresses #579
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
Checklist for assignee