Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- Mask boundary nodes in spatial loss maps computed during `test_step`, consistent with all other loss calls that use `interior_mask_bool` [\#568](https://github.com/mllam/neural-lam/pull/568) @RajdeepKushwaha5

- Standardize all script references to use `create_graph` instead of the legacy `create_mesh` name in README and `pyproject.toml`, and fix minor README typos [\#426](https://github.com/mllam/neural-lam/pull/426) @GiGiKoneti

- Initialize `da_forcing_mean` and `da_forcing_std` to `None` when forcing data is absent, fixing `AttributeError` in `WeatherDataset` with `standardize=True` [\#369](https://github.com/mllam/neural-lam/issues/369) @Sir-Sloth-The-Lazy
Expand Down
4 changes: 3 additions & 1 deletion neural_lam/models/ar_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,8 @@ def test_step(self, batch, batch_idx):
spatial_loss = self.loss(
prediction, target, pred_std, average_grid=False
) # (B, pred_steps, num_grid_nodes)
# Exclude boundary nodes, consistent with training/validation loss

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File is gone after the #208 refactor, this block should land in models/module.py:442 right after spatial_loss = self.loss(...):

        # Exclude boundary nodes, consistent with the loss used in
        # training/validation. NaN-mask in place so the tensor keeps its
        # (B, pred_steps, num_grid_nodes) shape for downstream plotting,
        # and is reduced with `torch.nanmean` in `on_test_epoch_end`.
        spatial_loss[..., ~self.interior_mask_bool] = float("nan")

spatial_loss[..., ~self.interior_mask_bool] = float("nan")
Comment on lines 447 to +451

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage: this change introduces NaN-masking for boundary nodes and switches the aggregation to nanmean, but there doesn’t appear to be a unit/integration test asserting that boundary nodes are excluded from the saved/plot-ready spatial loss maps. Adding a small test (e.g., with a minimal module/mocked masks similar to existing ARModel tests) would prevent regressions in evaluation outputs.

Copilot uses AI. Check for mistakes.
log_spatial_losses = spatial_loss[
:, [step - 1 for step in self.args.val_steps_to_log]
]
Expand Down Expand Up @@ -753,7 +755,7 @@ def on_test_epoch_end(self):
torch.cat(self.spatial_loss_maps, dim=0)
) # (N_test, N_log, num_grid_nodes)
if self.trainer.is_global_zero:
mean_spatial_loss = torch.mean(
mean_spatial_loss = torch.nanmean(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, apply at models/module.py:722:

            mean_spatial_loss = torch.nanmean(spatial_loss_tensor, dim=0)

spatial_loss_tensor, dim=0
) # (N_log, num_grid_nodes)

Expand Down
47 changes: 47 additions & 0 deletions tests/test_training.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,3 +189,50 @@ def all_gather(self, tensor, sync_grads=False):
"all_gather_cat produced incorrectly ordered/combined values "
"on multi-device simulation"
)

@sadamov sadamov Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop this one. It only verifies that torch.nanmean ignores NaN, which is testing PyTorch behavior rather than this integration. Could you replace it with an integration test that builds a ForecasterModule with a known interior_mask, runs trainer.test() end-to-end, and asserts boundary nodes are NaN in the saved mean_spatial_loss.pt? That would also be the first real coverage for test_step / on_test_epoch_end in this repo.


def test_spatial_loss_maps_exclude_boundary():
"""
Test that spatial loss maps mask out boundary nodes with NaN and that
nanmean correctly ignores them during aggregation, matching the masking
logic in test_step and on_test_epoch_end.
"""
num_grid_nodes = 10
pred_steps = 3
batch_size = 2

# Interior mask: first 6 nodes interior, last 4 boundary
interior_mask_bool = torch.tensor(
[True] * 6 + [False] * 4
) # (num_grid_nodes,)

# Simulate spatial loss (all ones for simplicity)
spatial_loss = torch.ones(batch_size, pred_steps, num_grid_nodes)

# Apply the same masking as test_step
spatial_loss[..., ~interior_mask_bool] = float("nan")

# Boundary nodes should be NaN
assert torch.all(
torch.isnan(spatial_loss[..., ~interior_mask_bool])
), "Boundary nodes should be NaN"

# Interior nodes should not be NaN
assert not torch.any(
torch.isnan(spatial_loss[..., interior_mask_bool])
), "Interior nodes should not be NaN"

# nanmean over batch dim should ignore NaN boundary nodes
mean_spatial_loss = torch.nanmean(spatial_loss, dim=0)
assert mean_spatial_loss.shape == (pred_steps, num_grid_nodes)

# Interior nodes: mean of 1.0 = 1.0
assert torch.allclose(
mean_spatial_loss[:, interior_mask_bool],
torch.ones(pred_steps, interior_mask_bool.sum().item()),
)

# Boundary nodes: nanmean of all-NaN = NaN
assert torch.all(
torch.isnan(mean_spatial_loss[:, ~interior_mask_bool])
), "Boundary nodes should remain NaN after nanmean"
Loading