Skip to content

closes #2214 Fix batch-dimension slicing in physicsnemo_fdm_swe_nonlin PDE residuals for batchsize=1 - #2233

Open
dmitriySem wants to merge 2 commits into
NVIDIA:mainfrom
dmitriySem:main
Open

dmitriySem wants to merge 2 commits into
NVIDIA:mainfrom
dmitriySem:main

Conversation

@dmitriySem

Copy link
Copy Markdown

closes #2214

Problem

In examples/cfd/swe_nonlinear_pino/train_utils/losses.py, the function physicsnemo_fdm_swe_nonlin slices the PDE residual tensors Dh, Du, and Dv along the batch dimension using [1:-1]. With the default batchsize: 1 in the example config, this produces an empty tensor (shape (0, ...)), causing loss_pde to become NaN and the physics loss gradient to be zero. Training proceeds silently without any physics contribution.

This is inconsistent with the reference function fdm_swe_nonlin in the same file, which correctly slices along the last (spatial) dimension using [..., 1:-1].

Fix

Removed the batch-dimension slicing from physicsnemo_fdm_swe_nonlin:

- Dh = pde_Dh["Dh"][1:-1]
- Du = pde_Du["Du"][1:-1]
- Dv = pde_Dv["Dv"][1:-1]
+ Dh = pde_Dh["Dh"]
+ Du = pde_Du["Du"]
+ Dv = pde_Dv["Dv"]

The [1:-1] slice was applied along the batch dimension, making the PDE
residual tensor empty for batchsize=1 and causing loss_pde to be NaN.
Remove the batch-dim slice so the physical loss is computed correctly."

Signed-off-by: Dmitriy <dvsemen93@gmail.com>
…iduals in the shallow water example, which caused `loss_pde` to be `NaN` for `batchsize=1`

Signed-off-by: Dmitriy <dvsemen93@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Nov 5, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

CODEOWNERS review map

Current for commit 0a352e31260c. An approval covers every file listed for that owner; one owner is sufficient for shared files.

No CODEOWNER

  • CHANGELOG.md
  • examples/cfd/swe_nonlinear_pino/train_utils/losses.py

Comment /codeowners-info to refresh.

@greptile-apps

greptile-apps Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in Greptile

[Medium impact] The change affects the shallow-water PINO example's PhysicsNeMo training path.

The fix appears safe to merge, with a non-blocking request for regression coverage.

Findings

  1. P2 Batch fix lacks regression coverage ▶

Summary

Fixes the shallow-water physics loss by keeping every batch element in Dh, Du, and Dv.

  • Shallow-water training keeps PDE residuals from one-item batches.

Reviews (1) · Last reviewed commit: "Fixed batch-dimension slicing in `physic..." · Reviewed by Greptile

Comment on lines +297 to +299
Dh = pde_Dh["Dh"]
Du = pde_Du["Du"]
Dv = pde_Dv["Dv"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Batch fix lacks regression coverage

The batch-slicing fix has no regression test. Add a small test for physicsnemo_fdm_swe_nonlin and physicsnemo_fourier with batch sizes 1, 2, and 3. Check that each residual keeps every batch element and that a nonconstant input produces a finite loss and finite, nonzero gradients. This would catch both empty residuals for small batches and silently dropped samples for larger batches.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the concrete suggestion. I agree that a regression test would be valuable. Since this is my first contribution to PhysicsNeMo and the current PR is intentionally minimal, I'd like to get feedback from the project maintainers first. If a test is required for acceptance, I'll add it to this PR or prepare a separate one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense. My comment was a coverage suggestion, not a claim that a regression test is required for acceptance. Keeping this PR focused and waiting for maintainer feedback is reasonable; if they request coverage, you can add it here or in a follow-up.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

This branch has not been deployed

No deployments
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.

swe_nonlinear_pino: physicsnemo_fdm_swe_nonlin slices batch dim ([1:-1]) -> empty PDE residual / NaN loss_pde with default batchsize=1

1 participant