Fix wrong lead-time unit in plot_error_heatmap axis label by nikhil3495 · Pull Request #743 · mllam/neural-lam

nikhil3495 · 2026-09-04T11:51:23Z

Describe your changes

plot_error_heatmap (neural_lam/vis.py) built the lead-time x-axis label from time_step_unit[0] - the first character of the unit word returned by utils.get_integer_time (weeks/days/hours/minutes/seconds/milliseconds/microseconds/unknown). That is only correct for weeks/days/hours/seconds:

minutes -> Lead time (m) (should be min; m reads as metres/months)
unknown (step length not an exact whole number of seconds) -> Lead time (u) (meaningless)
milliseconds / microseconds -> Lead time (m)

Every other call site (neural_lam/models/module.py:780, :1019) uses the full unit word; only this one truncated.

This maps the full unit name to a proper abbreviation via a module-level _LEAD_TIME_UNIT_ABBREVIATIONS dict, and labels the unknown case steps since the tick values are then plain step indices. Display-only change - no numbers change.

Added a parametrized test over hours/days/minutes/seconds/unknown step lengths and a drift-guard test that every unit name get_integer_time can return has an abbreviation.

Testing / verification

pre-commit run --all-files passes.
pytest tests/test_plotting.py --doctest-modules neural_lam/vis.py passes, including the new parametrized abbreviation test and the drift-guard test.
Manually inspected plot_error_heatmap output for minutes- and unknown-unit datasets to confirm the axis label now reads Lead time (min) / Lead time (steps) instead of the truncated single-letter label.

Issue Link

closes #742

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (Addition or improvements to documentation)

Checklist before requesting a review

My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
I have performed a self-review of my code
For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
I have updated the README to cover introduced code changes (not applicable - no user-facing API change)
I have added tests that prove my fix is effective or that my feature works
I have given the PR a name that clearly describes the change, written in imperative form (context).
I have requested a reviewer and an assignee (assignee is responsible for merging). This applies only if you have write access to the repo, otherwise feel free to tag a maintainer to add a reviewer and assignee. - no write access; tagging a maintainer

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

the code is readable
the code is well tested
the code is documented (including return types and parameters)
the code is easy to maintain

Author checklist after completed review

I have added a line to the CHANGELOG describing this change, in a section
reflecting type of change (add section where missing):
- added: when you have added new functionality
- changed: when default behaviour of the code has been changed
- fixes: when your contribution fixes a bug
- maintenance: when your contribution is relates to repo maintenance, e.g. CI/CD or documentation

Checklist for assignee

PR is up to date with the base branch
the tests pass
(if the PR is not just maintenance/bugfix) the PR is assigned to the next milestone. If it is not, propose it for a future milestone.
author has added an entry to the changelog (and designated the change as added, changed, fixed or maintenance)
Once the PR is ready to be merged, squash commits and merge the PR.

Generated with Claude Code

`plot_error_heatmap` derived the x-axis unit from `time_step_unit[0]`, which is only correct for weeks/days/hours/seconds. `minutes`, `milliseconds` and `microseconds` all rendered as "m", and `unknown` (no unit divides the step length evenly) rendered as "u". Map the full unit name from `get_integer_time` to a proper abbreviation and label the `unknown` case "steps", since the tick values are then plain step indices. Matches how `models/module.py` uses the full unit word everywhere else. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RAKmdjUwG7hKkRwnkW8obq

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RAKmdjUwG7hKkRwnkW8obq

observingClouds

Thanks for your contribution @nikhil3495. I think this is a valid fix and have only a few minor comments.

observingClouds · 2026-09-08T06:59:44Z

+# Short forms of the unit names returned by ``utils.get_integer_time`` for use
+# in axis labels. ``"minutes"``, ``"milliseconds"`` and ``"microseconds"`` all
+# start with "m", so the first character alone is ambiguous. ``"unknown"`` (no
+# unit divides the step length evenly) has no abbreviation; the tick labels are
+# then plain step indices, so "steps" is the honest label.


Please delete this comment, the dict is self-explanatory.

observingClouds · 2026-09-08T07:09:06Z

+    """Lead-time axis label uses the full unit abbreviation, not just its
+    first character (``minutes`` used to render as "m"), and reads "steps"
+    when no unit divides the step length evenly (used to render as "u")."""


Please only describe what it currently tests for, not what the previous behavior has been.

Suggested change

"""Lead-time axis label uses the full unit abbreviation, not just its

first character (``minutes`` used to render as "m"), and reads "steps"

when no unit divides the step length evenly (used to render as "u")."""

"""Check lead-time axis label uses unit abbreviation as given in ``_LEAD_TIME_UNIT_ABBREVIATIONS`` and uses consistent ticks."""

observingClouds · 2026-09-08T07:19:39Z

+def test_lead_time_unit_abbreviations_cover_get_integer_time_units():
+    """Every unit name ``get_integer_time`` can return has an abbreviation, so
+    the lead-time axis label never falls through to the raw word."""
+    get_integer_time_units = {
+        "weeks",
+        "days",
+        "hours",
+        "minutes",
+        "seconds",
+        "milliseconds",
+        "microseconds",
+        "unknown",
+    }
+    assert get_integer_time_units <= set(vis._LEAD_TIME_UNIT_ABBREVIATIONS)
+
+


Which case should this test cover? I think it is not very effective as the get_integer_time function can change without the unit list in this test being updated. I would delete this test.

Suggested change

def test_lead_time_unit_abbreviations_cover_get_integer_time_units():

"""Every unit name ``get_integer_time`` can return has an abbreviation, so

the lead-time axis label never falls through to the raw word."""

get_integer_time_units = {

"weeks",

"days",

"hours",

"minutes",

"seconds",

"milliseconds",

"microseconds",

"unknown",

}

assert get_integer_time_units <= set(vis._LEAD_TIME_UNIT_ABBREVIATIONS)

- Remove the explanatory comment above _LEAD_TIME_UNIT_ABBREVIATIONS; the dict is self-explanatory. - Reword test_plot_error_heatmap_lead_time_axis_label docstring. - Delete test_lead_time_unit_abbreviations_cover_get_integer_time_units, which was flagged as not an effective test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UWja96C3kPvPGsSijgayNV

observingClouds

This looks good now. Could you please use the full PR template? Currently a few sections are missing and those are needed to finish the PR process.

nikhil3495 · 2026-09-09T07:13:55Z

Sure

…

On Wed, 9 Sep 2026 at 12:24 PM, Hauke Schulz ***@***.***> wrote: ***@***.**** commented on this pull request. This looks good now. Could you please use the full PR template? Currently a few sections are missing and those are needed to finish the PR process. — Reply to this email directly, view it on GitHub <#743?email_source=notifications&email_token=BK6WJTUKDJ3OLLVIWPMYYML5OD5BRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJVGA3TOMJWHAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-5150771683>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BK6WJTU4IJABIE7PRDLJGM35OD5BRAVCNFSNUABFKJSXA33TNF2G64TZHM3DSNJZGA2DOMJRHNEXG43VMU5TKMZUG43TENBSGYZKC5QC> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/BK6WJTRIYDP7U6YEZKI5IVD5OD5BRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJVGA3TOMJWHAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android <https://github.com/notifications/mobile/android/BK6WJTTFMFWEGH5YVANGDBL5OD5BRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJVGA3TOMJWHAZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>

observingClouds · 2026-09-11T08:14:17Z

@nikhil3495 if you make the PR template changes before Monday, we can propose it for the next release.

nikhil3495 · 2026-09-11T10:13:29Z

Hi @observingClouds! I've updated the PR description with the complete
template — added the reviewer and assignee checklist sections, and a
testing/verification subsection. Let me know if anything else is needed
before Monday!

nikhil3495 · 2026-09-12T08:41:45Z

Thank you for the review! Let me know if anything else is needed.

nikhil3495 and others added 2 commits September 4, 2026 17:11

Fill in PR number in CHANGELOG entry

d2e90ee

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RAKmdjUwG7hKkRwnkW8obq

observingClouds requested changes Sep 8, 2026

View reviewed changes

observingClouds self-assigned this Sep 8, 2026

observingClouds reviewed Sep 9, 2026

View reviewed changes

observingClouds approved these changes Sep 12, 2026

View reviewed changes

observingClouds added bug Something isn't working ready Review complete - proposed for milestone labels Sep 12, 2026

nikhil3495 mentioned this pull request Sep 12, 2026

Fix silent stat corruption in distributed standardization-stats depadding #748

Open

21 tasks

Navigation Menu

Uh oh!

Fix wrong lead-time unit in plot_error_heatmap axis label - #743

Fix wrong lead-time unit in plot_error_heatmap axis label#743
nikhil3495 wants to merge 3 commits into
mllam:mainfrom
nikhil3495:fix/heatmap-lead-time-unit-abbreviation

nikhil3495 commented Sep 4, 2026 •

edited by observingClouds

Loading

Uh oh!

observingClouds left a comment

Uh oh!

observingClouds Sep 8, 2026

Uh oh!

observingClouds Sep 8, 2026

Uh oh!

observingClouds Sep 8, 2026

Uh oh!

observingClouds left a comment

Uh oh!

nikhil3495 commented Sep 9, 2026 via email

Uh oh!

observingClouds commented Sep 11, 2026

Uh oh!

nikhil3495 commented Sep 11, 2026

Uh oh!

nikhil3495 commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nikhil3495 commented Sep 4, 2026 • edited by observingClouds Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Testing / verification

Issue Link

Type of change

Checklist before requesting a review

Checklist for reviewers

Author checklist after completed review

Checklist for assignee

Uh oh!

observingClouds left a comment

Choose a reason for hiding this comment

Uh oh!

observingClouds Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

observingClouds Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

observingClouds Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

observingClouds left a comment

Choose a reason for hiding this comment

Uh oh!

nikhil3495 commented Sep 9, 2026 via email

Uh oh!

observingClouds commented Sep 11, 2026

Uh oh!

nikhil3495 commented Sep 11, 2026

Uh oh!

nikhil3495 commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nikhil3495 commented Sep 4, 2026 •

edited by observingClouds

Loading