Skip to content

Conversation

@saikrishnanc-nv
Copy link
Collaborator

PhysicsNeMo Pull Request

Description

This PR integrates PhysicsNeMo-Curator instructions to the Crash example.

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

N/A

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.

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.

Greptile Overview

Greptile Summary

This PR integrates PhysicsNeMo-Curator preprocessing instructions into the Crash example documentation and enhances the VTP reader to support additional point data fields (thickness, etc.).

Key Changes:

  • Added comprehensive documentation section explaining how to use PhysicsNeMo-Curator to preprocess LS-DYNA d3plot files into VTP format
  • Enhanced load_vtp_file() to return a third value point_data_dict containing non-displacement fields like thickness
  • Updated process_vtp_data() to propagate point data through records using record.update(point_data_dict)
  • Added comprehensive test suite with 8 test functions covering VTP loading, displacement application, connectivity extraction, and edge building

Issues Found:

  • Critical dtype mismatch in test suite: uses np.uint8 for coordinates, displacements, and thickness, which will cause data truncation and type inconsistencies with production code (expects np.float64 for coords, np.float32 for features)

Confidence Score: 2/5

  • This PR has critical dtype bugs in the test suite that will cause test failures and incorrect validation
  • The documentation and vtp_reader.py changes are solid (5/5), but the test suite has systematic dtype errors using np.uint8 instead of np.float64/float32 throughout. This will cause data loss, type mismatches with production code expectations, and potential test failures. The logic is correct but the implementation has critical flaws that must be fixed before merge.
  • examples/structural_mechanics/crash/tests/test_vtp_reader.py requires immediate attention - all dtype declarations need correction

Important Files Changed

File Analysis

Filename Score Overview
examples/structural_mechanics/crash/README.md 5/5 added comprehensive documentation for PhysicsNeMo-Curator integration, data preprocessing steps, input/output formats
examples/structural_mechanics/crash/vtp_reader.py 5/5 enhanced load_vtp_file to return additional point data (thickness, etc.), updated process_vtp_data to propagate point data through records
examples/structural_mechanics/crash/tests/test_vtp_reader.py 1/5 new comprehensive test suite for VTP reader, but uses incorrect np.uint8 dtypes causing data loss - should use np.float64 for coords/displacements and np.float32 for features

Sequence Diagram

sequenceDiagram
    participant User
    participant Curator as PhysicsNeMo-Curator
    participant VTPReader as vtp_reader.py
    participant Datapipe as datapipe.py
    participant Model as ML Model

    User->>Curator: physicsnemo-curator-etl<br/>(d3plot files)
    Curator->>Curator: Parse LS-DYNA d3plot<br/>Extract displacements<br/>Extract thickness from .k files
    Curator->>VTPReader: Generate .vtp files<br/>(displacement_t*, thickness)
    
    User->>VTPReader: load_vtp_file(vtp_path)
    VTPReader->>VTPReader: Read reference coords
    VTPReader->>VTPReader: Extract displacement_t* fields
    VTPReader->>VTPReader: Compute absolute positions<br/>(coords + displacement)
    VTPReader->>VTPReader: Extract mesh connectivity
    VTPReader->>VTPReader: Extract point_data_dict<br/>(thickness, etc.)
    VTPReader-->>Datapipe: Return (pos_raw, connectivity, point_data_dict)
    
    Datapipe->>Datapipe: Build records with<br/>coords + features
    Datapipe->>Datapipe: Normalize data
    Datapipe->>Datapipe: Create SimSample<br/>(x['coords'], x['features'], y)
    Datapipe-->>Model: Training-ready tensors
Loading

3 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

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.

Greptile Overview

Greptile Summary

Adds comprehensive test suite for the Crash example's VTP reader module, addressing all critical dtype issues from previous review.

  • Creates fixture-based tests using pyvista to generate synthetic VTP files
  • Tests core functionality: file loading, displacement application, mesh connectivity extraction, and edge building
  • Validates that displacement fields are correctly excluded from point_data_dict while other fields (like thickness) are included
  • Includes edge case tests for missing displacement fields and empty connectivity
  • All numpy arrays now use correct dtypes matching production code (float64 for coordinates/displacements, float32 for thickness)

Confidence Score: 5/5

  • This PR is safe to merge with no remaining issues
  • All critical dtype issues from previous review have been correctly addressed. The test suite is well-structured, comprehensive, and follows pytest best practices with proper fixtures and cleanup
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
examples/structural_mechanics/crash/tests/test_vtp_reader.py 5/5 New comprehensive test suite for VTP reader functionality with corrected dtypes after addressing previous review feedback

Sequence Diagram

sequenceDiagram
    participant Test as Test Suite
    participant Fixture as simple_vtp_file fixture
    participant PyVista as PyVista
    participant VTPReader as vtp_reader module
    participant File as Temp VTP File

    Test->>Fixture: Request test fixture
    Fixture->>PyVista: Create PolyData with points (float64)
    Fixture->>PyVista: Add faces (quad cell)
    Fixture->>PyVista: Add displacement_t0.000 (float64)
    Fixture->>PyVista: Add displacement_t0.005 (float64)
    Fixture->>PyVista: Add displacement_t0.010 (float64)
    Fixture->>PyVista: Add thickness point data (float32)
    Fixture->>File: Save to temp .vtp file
    Fixture-->>Test: Return file path

    Test->>VTPReader: load_vtp_file(path)
    VTPReader->>PyVista: pv.read(vtp_path)
    PyVista-->>VTPReader: Return PolyData
    VTPReader->>VTPReader: Extract coords (float64)
    VTPReader->>VTPReader: Find displacement fields
    VTPReader->>VTPReader: Apply displacements to coords
    VTPReader->>VTPReader: extract_mesh_connectivity_from_polydata()
    VTPReader->>VTPReader: Extract non-displacement point data
    VTPReader-->>Test: Return (pos_raw, connectivity, point_data_dict)

    Test->>Test: Assert shapes and values
    Test->>VTPReader: extract_mesh_connectivity_from_polydata()
    VTPReader-->>Test: Return connectivity list
    Test->>VTPReader: build_edges_from_mesh_connectivity()
    VTPReader-->>Test: Return edge set

    Test->>Fixture: Cleanup
    Fixture->>File: Delete temp file
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@saikrishnanc-nv saikrishnanc-nv self-assigned this Nov 7, 2025
@mnabian
Copy link
Collaborator

mnabian commented Nov 10, 2025

Thanks Sai for the PR! It looks good to me, although I suggest we hold this until the other curator PR is also reviewed. We need to also think about whether we should keep the current d3plot reader or not, a discussion we should probably have with Ram.

@saikrishnanc-nv
Copy link
Collaborator Author

Thanks Sai for the PR! It looks good to me, although I suggest we hold this until the other curator PR is also reviewed. We need to also think about whether we should keep the current d3plot reader or not, a discussion we should probably have with Ram.

@mnabian Thanks for the review Mohammad! The PR in Curator has been merged. Can you approve this PR, and then we can remove the d3plot reader if Ram feels so? It should be easy to remove, since it's supported by default in Curator now.

@saikrishnanc-nv
Copy link
Collaborator Author

/blossom-ci

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 12, 2025

Greptile Overview

Greptile Summary

Integrates PhysicsNeMo-Curator preprocessing workflow into the Crash example by adding comprehensive documentation and enhancing the VTP reader to extract additional point data fields (thickness, etc.).

Key changes:

  • Added detailed Curator documentation to README covering installation, data structure requirements, and VTP format specifications
  • Enhanced load_vtp_file to return a point_data_dict containing non-displacement fields like thickness
  • Updated process_vtp_data to propagate point data fields to output records for use by the datapipe
  • Added comprehensive test coverage for VTP reader functionality

Note: The test fixture uses np.uint8 dtype for test data, which has already been flagged in previous comments as causing potential data loss and dtype mismatches with production code expectations.

Confidence Score: 4/5

  • Safe to merge with minor test data quality issues already flagged
  • The core functionality changes are well-implemented and backward-compatible. Documentation additions are helpful and accurate. The only concern is the test fixture dtype issue (np.uint8) which has been thoroughly documented in previous comments and should be addressed separately to ensure tests accurately reflect production data characteristics.
  • The test file examples/structural_mechanics/crash/tests/test_vtp_reader.py requires dtype corrections (already flagged in previous comments) to ensure test data matches production expectations

Important Files Changed

File Analysis

Filename Score Overview
examples/structural_mechanics/crash/README.md 5/5 Added comprehensive PhysicsNeMo-Curator documentation with installation instructions, data structure requirements, and VTP format details - purely documentation changes with no code impact
examples/structural_mechanics/crash/vtp_reader.py 5/5 Enhanced load_vtp_file to return point data dictionary (e.g., thickness) and updated process_vtp_data to include this data in output records - clean implementation that preserves backward compatibility
examples/structural_mechanics/crash/tests/test_vtp_reader.py 3/5 New test file with comprehensive coverage for VTP reader functionality - however, fixture uses np.uint8 dtype which causes data inconsistencies (already flagged in previous comments)

Sequence Diagram

sequenceDiagram
    participant User
    participant Curator as PhysicsNeMo-Curator
    participant VTPReader as vtp_reader.py
    participant DataPipe as datapipe.py
    participant Model as Training/Inference

    User->>Curator: Run physicsnemo-curator-etl
    Note over Curator: Process d3plot files
    Curator->>Curator: Extract mesh coordinates
    Curator->>Curator: Extract displacement fields per timestep
    Curator->>Curator: Extract thickness from .k files
    Curator-->>User: Generate VTP files with:<br/>- Reference coords<br/>- displacement_t* fields<br/>- thickness data

    User->>VTPReader: Load VTP file via Reader()
    VTPReader->>VTPReader: load_vtp_file(vtp_path)
    VTPReader->>VTPReader: Read poly.points as coords (float64)
    VTPReader->>VTPReader: Sort displacement_t* fields
    VTPReader->>VTPReader: Compute pos_raw = coords + displacements
    VTPReader->>VTPReader: Extract mesh_connectivity from faces
    VTPReader->>VTPReader: Extract point_data_dict (thickness, etc.)
    VTPReader-->>DataPipe: Return (srcs, dsts, point_data)<br/>point_data = {"coords": [T,N,3], "thickness": [N]}

    DataPipe->>DataPipe: Convert coords to float32 tensors
    DataPipe->>DataPipe: Concatenate features (thickness, etc.) as float32
    DataPipe->>DataPipe: Normalize positions and features
    DataPipe-->>Model: Provide x={'coords': [N,3], 'features': [N,F]}<br/>and targets y: [N,(T-1)*3]

    Model->>Model: Train or infer on crash dynamics
Loading

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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@mnabian
Copy link
Collaborator

mnabian commented Nov 12, 2025

/blossom-ci

@saikrishnanc-nv saikrishnanc-nv force-pushed the saikrishnanc/update-crash branch from 727df69 to a9a9c0a Compare November 12, 2025 18:32
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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@saikrishnanc-nv
Copy link
Collaborator Author

/blossom-ci

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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@mnabian
Copy link
Collaborator

mnabian commented Nov 12, 2025

/blossom-ci

Copy link
Collaborator

@mnabian mnabian left a comment

Choose a reason for hiding this comment

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

LGTM!

@saikrishnanc-nv saikrishnanc-nv merged commit 8252271 into NVIDIA:main Nov 12, 2025
1 check passed
@saikrishnanc-nv saikrishnanc-nv deleted the saikrishnanc/update-crash branch November 12, 2025 20:16
coreyjadams added a commit that referenced this pull request Nov 14, 2025
* Move filesystems and version_check to core

* Fix version check tests

* Reorganize distributed, domain_parallel, and begin nn / utils cleanup.

* Move modules and meta to core.  Move registry to core.

No tests fixed yet.

* Add missing init files

* Update build system and specify some deps.

* Reorganize tests.

* Update init files

* Clean up neighbor tools.

* Update testing

* Fix compat tests

* Move core model tests to tests/core/

* Add import lint config

* Relocate layers

* Move graphcast utils into model directory

* Relocating util functionalities.

* Further clean up and organize tests.

* utils tests are passing now

* Cleaning up distributed tests

* Patching tests working again in nn

* Fix sdf test

* Fix zenith angle tests

* Some organization of tests.  Checkpoints is moved into utils.

* Remove launch.utils and launch.config.  Checkpointing is moved to
phsyicsnemo.utils, launch.config is just gone.  It was empty.

* Most nn tests are passing

* Further cleanup.  Getting there!

* Remove constants file

* Add import linting to pre-commit.

* Update crash readme (#1212)

* update license headers- second try

* update readme

* Bump multi-storage-client to v0.33.0 with rust client (#1156)

* Move gnn layers and start to fix several model tests.

* AFNO is now passing.

* Rnn models passing.

* Fix improt

* Healpix tests are working

* Domino and unet working

* Add jaxtyping to requirements.txt for crash sample (#1218)

* update license headers- second try

* Update requirements.txt

* Updating to address some test issues

* Replace 'License' link with 'Dev blog' link (#1215)

Co-authored-by: Corey adams <[email protected]>

* MGN tests passing again

* Most graphcast tests passing again

* Move nd conv layers.

* update fengwu and pangu

* Update sfno and pix2pix test

* update tests for figconvnet, swinrnn, superresnet

* updating more models to pass

* Update distributed tests, now passing.

* Validation fu added to examples/structural_mechanics/crash/train.py (#1204)

* validation added: works for multi-node job.

* rename and rearrange validation function

* validate_every_n_epochs, save_ckpt_every_n_epochs added in config

* corrected bug (args of model) in inference

* args in validation code updated

* val path added and args name changed

* validation split added -> write_vtp=False

* fixed inference bug

* bug fix: write_vtp

* Domain parallel tests now passing.

* Fix active learning imports so tests pass in refactor

* Fix some metric imports

* Remove deploy package

* Remove unused test file

* unmigrate these files ... again?

* Update import linter.

* Add saikrishnanc-nv to github actors (#1225)

* Integrate Curator instructions to the Crash example (#1213)

* Integrate Curator instructions

* Update docs

* Formatting changes

* Adding code of conduct (#1214)

* Adding code of conduct

Adopting the code of conduct from the https://www.contributor-covenant.org/

* Update CODE_OF_CONDUCT.MD

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

* Create .markdownlintignore

* Revise README for PhysicsNeMo resources and guidance

Updated the 'Getting Started' section and added new resources for learning AI Physics.

* Update README.md

---------

Co-authored-by: Mohammad Amin Nabian <[email protected]>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Corey adams <[email protected]>

* Cleaning up diffusion models. Not quite done yet.

* Restore deleted files

* Updating more tests.

* Further updates to tests.  Datapipes almost working.

---------

Co-authored-by: Mohammad Amin Nabian <[email protected]>
Co-authored-by: Yongming Ding <[email protected]>
Co-authored-by: ram-cherukuri <[email protected]>
Co-authored-by: Deepak Akhare <[email protected]>
Co-authored-by: Sai Krishnan Chandrasekar <[email protected]>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
coreyjadams added a commit that referenced this pull request Nov 18, 2025
* Move filesystems and version_check to core

* Fix version check tests

* Reorganize distributed, domain_parallel, and begin nn / utils cleanup.

* Move modules and meta to core.  Move registry to core.

No tests fixed yet.

* Add missing init files

* Update build system and specify some deps.

* Reorganize tests.

* Update init files

* Clean up neighbor tools.

* Update testing

* Fix compat tests

* Move core model tests to tests/core/

* Add import lint config

* Relocate layers

* Move graphcast utils into model directory

* Relocating util functionalities.

* Further clean up and organize tests.

* utils tests are passing now

* Cleaning up distributed tests

* Patching tests working again in nn

* Fix sdf test

* Fix zenith angle tests

* Some organization of tests.  Checkpoints is moved into utils.

* Remove launch.utils and launch.config.  Checkpointing is moved to
phsyicsnemo.utils, launch.config is just gone.  It was empty.

* Most nn tests are passing

* Further cleanup.  Getting there!

* Remove constants file

* Add import linting to pre-commit.

* Update crash readme (#1212)

* update license headers- second try

* update readme

* Bump multi-storage-client to v0.33.0 with rust client (#1156)

* Move gnn layers and start to fix several model tests.

* AFNO is now passing.

* Rnn models passing.

* Fix improt

* Healpix tests are working

* Domino and unet working

* Add jaxtyping to requirements.txt for crash sample (#1218)

* update license headers- second try

* Update requirements.txt

* Updating to address some test issues

* Replace 'License' link with 'Dev blog' link (#1215)

Co-authored-by: Corey adams <[email protected]>

* MGN tests passing again

* Most graphcast tests passing again

* Move nd conv layers.

* update fengwu and pangu

* Update sfno and pix2pix test

* update tests for figconvnet, swinrnn, superresnet

* updating more models to pass

* Update distributed tests, now passing.

* Validation fu added to examples/structural_mechanics/crash/train.py (#1204)

* validation added: works for multi-node job.

* rename and rearrange validation function

* validate_every_n_epochs, save_ckpt_every_n_epochs added in config

* corrected bug (args of model) in inference

* args in validation code updated

* val path added and args name changed

* validation split added -> write_vtp=False

* fixed inference bug

* bug fix: write_vtp

* Domain parallel tests now passing.

* Fix active learning imports so tests pass in refactor

* Fix some metric imports

* Remove deploy package

* Remove unused test file

* unmigrate these files ... again?

* Update import linter.

* Add saikrishnanc-nv to github actors (#1225)

* Integrate Curator instructions to the Crash example (#1213)

* Integrate Curator instructions

* Update docs

* Formatting changes

* Adding code of conduct (#1214)

* Adding code of conduct

Adopting the code of conduct from the https://www.contributor-covenant.org/

* Update CODE_OF_CONDUCT.MD

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

* Create .markdownlintignore

* Revise README for PhysicsNeMo resources and guidance

Updated the 'Getting Started' section and added new resources for learning AI Physics.

* Update README.md

---------

Co-authored-by: Mohammad Amin Nabian <[email protected]>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Corey adams <[email protected]>

* Cleaning up diffusion models. Not quite done yet.

* Restore deleted files

* Updating more tests.

* Fixed minor bug in shape validation in SongUNet (#1230)

Signed-off-by: Charlelie Laurent <[email protected]>

* Add Zarr reader for Crash (#1228)

* Add Zarr reader for Crash

* Update README

* Update validation logic of point data in Zarr reader

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

* Update examples/structural_mechanics/crash/zarr_reader.py

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

* Add a test for 2D feature arrays

* Update examples/structural_mechanics/crash/zarr_reader.py

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

---------

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

* Further updates to tests.  Datapipes almost working.

* update import paths

* Starting to clean up dependency tree.

---------

Signed-off-by: Charlelie Laurent <[email protected]>
Co-authored-by: Mohammad Amin Nabian <[email protected]>
Co-authored-by: Yongming Ding <[email protected]>
Co-authored-by: ram-cherukuri <[email protected]>
Co-authored-by: Deepak Akhare <[email protected]>
Co-authored-by: Sai Krishnan Chandrasekar <[email protected]>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Charlelie Laurent <[email protected]>
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