Skip to content

Some suggestions for the colormap scaling#1

Merged
kshirajahere merged 3 commits intokshirajahere:fix/-#375-Improvements-to-plot_error_map-functionfrom
sadamov:fix/explicit-normalization-parameter
Apr 17, 2026
Merged

Some suggestions for the colormap scaling#1
kshirajahere merged 3 commits intokshirajahere:fix/-#375-Improvements-to-plot_error_map-functionfrom
sadamov:fix/explicit-normalization-parameter

Conversation

@sadamov
Copy link
Copy Markdown

@sadamov sadamov commented Apr 17, 2026

This PR replaces the implicit 3-level fallback chain in _get_heatmap_color_values with an explicit normalization parameter on plot_error_heatmap. I think this should be a clear choice. But would argue that state_std is the better default than diff_std. There is no need however to expose this flag to the user I don't think.

plot_error_heatmap(errors, datastore, title=None, normalization="state_std")  # or "diff_std"
normalization Color value Required stat
"state_std" (default) Error / state_std state_std
"diff_std" Error / diff_std state_diff_std_standardized and state_std

Both modes fall back to per-variable max normalization when their required stat is missing (legacy behavior). Here I think the absolute error values between variables are just too different and the heatmap is not really meaningful at that point.

I had to update the docstrings and the tests accordingly.

sadamov and others added 3 commits April 17, 2026 08:22
…parameter

Adds `normalization: str = "state_std"` to `plot_error_heatmap` and
rewrites `_get_heatmap_color_values` to support two explicit modes:
- `"state_std"`: RMSE / state_std, white-to-red Reds colormap
- `"one_step"`: RMSE / diff_std, white-to-red Reds colormap

Both modes fall back to per-variable max (also Reds, [0,1]) when their
required stat is missing. The two modes never silently upgrade to each
other. Removes the custom `_HEATMAP_CMAP` in favour of the built-in
`"Reds"` colormap string. Updates tests accordingly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kshirajahere
Copy link
Copy Markdown
Owner

Thanks @sadamov. I reviewed this locally and I like the direction overall. Making the normalization explicit is definitely clearer than the previous silent fallback chain, and from the COSMO examples I also agree that state_std is the better default for cross-variable comparison, while diff_std still remains available explicitly when that view is wanted.

I reran the focused heatmap tests locally on this branch and they pass on my side.
Just two things I wanted to confirm before I merge:

  1. are you happy with diff_std remaining an explicit plot_error_heatmap(...) option for now, without plumbing it further through the current eval/logging path?
  2. and are you happy with the missing-stats fallback now being per-variable max normalization rather than the previous raw absolute-scale behavior?

If yes, I m happy to merge this into mllam#376.

@sadamov
Copy link
Copy Markdown
Author

sadamov commented Apr 17, 2026

2 * yes. :)

I think having a CLI flag just for this is too much. But having the explicit option in the code makes it easy to understand and change in research applications.

Absolute RMSE across variables (Pa vs K vs m/s) produces an unreadable colormap dominated by whichever variable has the largest physical units. Per-variable max at least preserves the relative evolution across lead times per variable.

@kshirajahere
Copy link
Copy Markdown
Owner

I agree with keeping the normalization choice as an explicit code-level option rather than a dedicated CLI flag, and I also agree that per-variable max is the better fallback than raw absolute RMSE across mixed-unit variables.
Also, I reran the focused heatmap tests locally on this branch and they pass on my side, so I m happy to merge this into mllam#376 ;)

@kshirajahere kshirajahere merged commit 11a925d into kshirajahere:fix/-#375-Improvements-to-plot_error_map-function Apr 17, 2026
3 of 5 checks passed
@sadamov sadamov deleted the fix/explicit-normalization-parameter branch April 17, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants