Skip to content

Conversation

@ktangsali
Copy link
Collaborator

PhysicsNeMo Pull Request

Description

The navier_stokes_rnn.py example's prepare_data function was incorrectly slicing 16 input timesteps when One2ManyRNN expects only 1 timestep—this bug was previously masked because the old model lacked shape validation and silently ignored extra timesteps. Fixed the data slicing logic to correctly prepare 1 input timestep for one2many mode and 16 input timesteps for seq2seq mode, aligning with the models' expected input shapes.

Checklist

Dependencies

Review Process

All PRs are reviewed by the PhysicsNeMo team before merging.

Depending on which files are changed, GitHub may automatically assign a maintainer for review.

We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.

AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 9, 2026

Greptile Overview

Greptile Summary

Fixed critical bug in prepare_data function where input timesteps were incorrectly sliced from the data array, causing shape mismatches with the RNN models.

  • Changed input slicing from input_nr_tsteps:input_nr_tsteps+predict_nr_tsteps to 0:input_nr_tsteps to correctly prepare 1 timestep for One2ManyRNN and 16 timesteps for Seq2SeqRNN
  • Changed output slicing from input_nr_tsteps+predict_nr_tsteps:input_nr_tsteps+2*predict_nr_tsteps to input_nr_tsteps:input_nr_tsteps+predict_nr_tsteps to get the correct prediction window
  • The bug was previously masked because the old model implementation lacked shape validation and silently ignored extra timesteps
  • The fix aligns with the models' input validation requirements: One2ManyRNN expects exactly 1 timestep (validated in rnn_one2many.py:241-245), while Seq2SeqRNN expects nr_tsteps timesteps (validated in rnn_seq2seq.py:242-247)

Important Files Changed

Filename Overview
examples/cfd/navier_stokes_rnn/navier_stokes_rnn.py Corrected data slicing in prepare_data to properly align input/output timesteps with model requirements (1 timestep for one2many, 16 for seq2seq)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@coreyjadams coreyjadams left a comment

Choose a reason for hiding this comment

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

LGTM

@ktangsali
Copy link
Collaborator Author

/blossom-ci

@ktangsali ktangsali enabled auto-merge February 10, 2026 21:19
@coreyjadams
Copy link
Collaborator

FYI failing doctest fixes are in the pipeline.

@ktangsali
Copy link
Collaborator Author

/blossom-ci

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.

3 participants