Skip to content

Conversation

@AbVishwas
Copy link

@AbVishwas AbVishwas commented Nov 22, 2025

PhysicsNeMo Pull Request

Description

Checklist

  • [] I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • [] The documentation is up to date with these changes.
  • The CHANGELOG.md is up to date with these changes.
  • An issue is linked to this pull request.

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.

Abhijeet Vishwasrao and others added 5 commits November 21, 2025 18:15
…mplete pipeline using EDM model

Signed-off-by: Abhijeet Vishwasrao <[email protected]>
Signed-off-by: Abhijeet Vishwasrao <[email protected]>
Signed-off-by: Abhijeet Vishwasrao <[email protected]>
Signed-off-by: Abhijeet Vishwasrao <[email protected]>
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 22, 2025

Greptile Overview

Greptile Summary

Adds complete pipeline for 2D urban turbulent flow generation using EDM diffusion models, including training, generation, and statistical evaluation capabilities.

Critical Issues Found:

  • Generation-evaluation pipeline broken: generate.py saves .npy files but evaluate-uncond-gen-2D.py expects HDF5 format
  • Undefined variable config in stats_helpers.py:1122 (should be self.ds_ratio)
  • README documents incorrect output format (claims HDF5, actually .npy)

Strengths:

  • Comprehensive implementation with training, generation, and evaluation scripts
  • Well-documented README with detailed usage instructions
  • Proper CHANGELOG entry
  • Good code structure with separate modules for datasets, helpers, and configurations
  • Includes visualization results showing model performance

Recommendations:

  • Fix the file format mismatch between generation and evaluation
  • Update README to match actual implementation or vice versa
  • Fix undefined variable bug in stats helpers
  • Consider adding a post-processing script to consolidate .npy files into HDF5

Important Files Changed

File Analysis

Filename Score Overview
examples/weather/diffusion-urban-flows-2D/generate.py 2/5 Generation script with critical bug: saves .npy files but evaluation expects HDF5 format
examples/weather/diffusion-urban-flows-2D/train.py 4/5 Training script implementing EDM diffusion model with distributed training support
examples/weather/diffusion-urban-flows-2D/evaluate-uncond-gen-2D.py 3/5 Evaluation script expecting HDF5 input that generate.py doesn't produce
examples/weather/diffusion-urban-flows-2D/helpers/stats_helpers.py 3/5 Statistical evaluation module with undefined variable bug in plot_probe_signal
examples/weather/diffusion-urban-flows-2D/README.md 3/5 Documentation with incorrect output format description

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.

22 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +356 to +360
for seed, pred_np in zip(batch_seeds, preds_np):
image_dir = img_outdir
os.makedirs(image_dir, exist_ok=True)
image_path = os.path.join(image_dir, f"{seed:06d}.npy")
np.save(image_path, pred_np)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Outputs saved as .npy files but evaluate-uncond-gen-2D.py:44 expects HDF5 format with key 'pred'. This breaks the evaluation pipeline.

Either consolidate individual .npy files into HDF5 after generation, or update evaluation to load from .npy files.

Comment on lines +300 to +317
Generated samples are saved as HDF5 files:

```text
outputs/diffusion_uflow/generated/pred_snaps-<steps>.h5
```

**File Structure:**

```python
{
'u_pred': (N, 288, 96), # Generated U-velocity
'v_pred': (N, 288, 96), # Generated V-velocity
'x': (288,), # X-coordinates
'y': (96,) # Y-coordinates
}
```

Velocities are denormalized to physical units.
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Incorrect output format documented. generate.py saves individual .npy files (line 359-360), not HDF5.

Comment on lines +1122 to +1133
ds_ratio=config.dataset.ds_ratio,
) # Check this while integrating the function
u_pt_p = get_data_for_stats(
self.pred[:, i],
x=x,
y=y,
z=z,
input_data_type="2D",
data="point",
mean_over_time=False,
ds_ratio=config.dataset.ds_ratio,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: config variable undefined - should be self.ds_ratio (used consistently elsewhere in this class)


# Generation
- generation/uflow2d
#- generation/patched_based
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Space missing before # comment character

@CharlelieLrt CharlelieLrt self-requested a review November 24, 2025 22:42
@CharlelieLrt CharlelieLrt self-assigned this Nov 24, 2025
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.

2 participants