Skip to content

step=0 or negative in --val_steps_to_log / --train_steps_to_log / --var_leads_metrics_watch silently logs the wrong step's loss #745

Description

@RudraDudhat2509

step > max_steps is the only bound checked at CLI parse time for --train_steps_to_log, --val_steps_to_log, and the per-variable lead times in --var_leads_metrics_watch. Nothing checks step >= 1.

Default is [1, 2, 3, 5, 10], so 1-indexing is clearly the intended scheme (there's no "step 0" in a 1-indexed unroll).

The actual bug is in ForecasterModule._log_step_loss:

for step in steps_to_log:
    if step <= len(time_step_loss):
        log_dict[f"{phase}_loss_unroll{step}"] = time_step_loss[step - 1]

step=0 passes step <= len(time_step_loss) and then indexes time_step_loss[-1], Python's negative-index wraparound, silently returning the last step's loss under the ..._unroll0 key. step=-1 returns time_step_loss[-2], and so on. Same pattern in test_step's spatial-loss indexing (spatial_loss[:, [step - 1 for step in val_steps_to_log if step <= spatial_loss.shape[1]]]).

So --val_steps_to_log 0 (an easy typo if someone is used to 0-indexing) doesn't error and doesn't get skipped, it logs real data under a misleading metric name.

Repro

from unittest.mock import MagicMock, patch
import loguru
loguru.logger.catch = lambda f: f
from neural_lam.train_model import main

mock_args = MagicMock()
mock_args.eval = None
mock_args.load = None
mock_args.config_path = "dummy.yaml"
mock_args.val_steps_to_log = [0]
mock_args.train_steps_to_log = []
mock_args.var_leads_metrics_watch = "{}"
mock_args.ar_steps_eval = 10
mock_args.ar_steps_train = 10

with patch("neural_lam.train_model.ArgumentParser.parse_args", return_value=mock_args):
    with patch("neural_lam.train_model.load_config_and_datastore", return_value=(MagicMock(), MagicMock())):
        main()  # does not raise, no error, no warning

Compare to --val_steps_to_log -1, -2, etc., same silent pass-through.

I have a fix ready (adds the missing step >= 1 check at CLI parse time, in the same two loops that already check the upper bound) and will open a PR shortly.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions