Extend crash datapipe, readers, and update README #1194
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the crash simulation datapipe to use a modular, configurable reader architecture. The core change introduces a Reader class in d3plot_reader.py that wraps the existing process_d3plot_data function, making data reading logic injectable via Hydra configuration. The CrashBaseDataset class now accepts a generic reader callable instead of hardcoding the d3plot processing function, enabling support for different data formats or reader implementations. Configuration files are restructured to separate reader settings (conf/reader/d3plot.yaml) from datapipe settings, and the wall_node_disp_threshold parameter is moved from datapipe configs to the reader config. The Reader class also implements split-aware behavior, automatically disabling VTP file generation during training to reduce I/O overhead. This design follows Hydra's compositional config pattern and improves separation of concerns by decoupling data reading from dataset instantiation logic.
PR Description Notes:
- The PR description is empty and none of the checklist items are completed, making it difficult to understand the motivation or verify completeness of the changes.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/d3plot_reader.py | 5/5 | Adds Reader wrapper class with configurable threshold and split-aware VTP writing |
| examples/structural_mechanics/crash/datapipe.py | 4/5 | Refactors CrashBaseDataset to accept injectable reader callable instead of hardcoded function |
| examples/structural_mechanics/crash/conf/config.yaml | 5/5 | Adds reader component to Hydra defaults composition for modular configuration |
| examples/structural_mechanics/crash/conf/reader/d3plot.yaml | 4/5 | New configuration file for d3plot reader with wall_node_disp_threshold parameter |
| examples/structural_mechanics/crash/train.py | 3/5 | Instantiates reader from config and passes to datapipe; contains redundant logging statements |
| examples/structural_mechanics/crash/conf/datapipe/graph.yaml | 4/5 | Removes wall_node_disp_threshold parameter as it's now handled by reader config |
| examples/structural_mechanics/crash/conf/datapipe/point_cloud.yaml | 3/5 | Removes wall_node_disp_threshold parameter as it's now handled by reader config |
Confidence score: 4/5
- This PR introduces a clean architectural improvement with well-structured modularization and should be safe to merge with minor concerns addressed.
- Score reflects solid design with clear separation of concerns, but deducted one point due to: (1) redundant
logging.getLogger().setLevel(logging.INFO)calls intrain.py(lines 70,78, 113) that appear to be debugging remnants, (2) unused parameters (wall_node_disp_thresholdandwrite_vtp) left indatapipe.pyconstructor signature that should be removed for clarity, and (3) the relative module path inconf/reader/d3plot.yaml(_target_: d3plot_reader.Reader) may cause import issues if run from incorrect directory. - Pay close attention to
examples/structural_mechanics/crash/train.pyto clean up the duplicate logging statements and verify the reader instantiation works correctly with the Hydra config structure across different execution contexts.
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant Trainer
participant Reader
participant Dataset
participant Model
participant Optimizer
User->>Hydra: "python train.py"
Hydra->>Hydra: "Load config.yaml"
Hydra->>main: "Initialize with config"
main->>DistributedManager: "Initialize distributed training"
main->>Trainer: "Create trainer instance"
Trainer->>Reader: "Instantiate d3plot reader"
Reader->>Reader: "Set wall_node_disp_threshold"
Trainer->>Dataset: "Instantiate datapipe (Graph or PointCloud)"
Dataset->>Reader: "Call reader(data_dir, num_samples, split)"
Reader->>Reader: "find_run_folders()"
loop For each run folder
Reader->>Reader: "load_d3plot_data()"
Reader->>Reader: "parse_k_file() for thickness"
Reader->>Reader: "compute_node_type() filter wall nodes"
Reader->>Reader: "build_edges_from_mesh_connectivity()"
Reader->>Reader: "collect_mesh_pos()"
end
Reader-->>Dataset: "Return srcs, dsts, point_data"
Dataset->>Dataset: "Convert to tensors (mesh_pos_seq, thickness_data)"
Dataset->>Dataset: "_compute_autoreg_node_stats()"
Dataset->>Dataset: "_compute_thickness_stats()"
Dataset->>Dataset: "Save stats to stats/ directory"
Dataset->>Dataset: "Normalize trajectories and thickness"
alt Graph Dataset
Dataset->>Dataset: "create_graph() for each sample"
Dataset->>Dataset: "add_edge_features()"
Dataset->>Dataset: "_compute_edge_stats()"
Dataset->>Dataset: "Normalize edge features"
end
Dataset-->>Trainer: "Return initialized dataset"
Trainer->>Trainer: "Create DataLoader with DistributedSampler"
Trainer->>Trainer: "Instantiate model (MeshGraphNet or Transolver)"
Trainer->>Trainer: "Setup optimizer, scheduler, scaler"
Trainer->>Trainer: "load_checkpoint() if exists"
loop For each epoch
Trainer->>Trainer: "sampler.set_epoch()"
loop For each batch
Trainer->>Dataset: "Get next sample"
Dataset-->>Trainer: "Return SimSample(node_features, node_target, graph)"
Trainer->>Trainer: "optimizer.zero_grad()"
Trainer->>Model: "forward(sample, data_stats)"
Model-->>Trainer: "Return predictions [T, N, 3]"
Trainer->>Trainer: "criterion(pred, target)"
Trainer->>Trainer: "backward() with AMP"
Trainer->>Optimizer: "step()"
end
Trainer->>Trainer: "scheduler.step()"
Trainer->>Trainer: "Log avg_loss to tensorboard"
Trainer->>Trainer: "save_checkpoint()"
end
Trainer-->>User: "Training completed!"
7 files reviewed, 4 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the crash datapipe to support configurable node features beyond just thickness. The main changes include:
- Modularized data reader: Extracted
Readerclass ind3plot_reader.pythat can be instantiated via Hydra config, movingwall_node_disp_thresholdto the reader config - Generic feature system: Replaced hardcoded thickness handling with a flexible
featureslist in datapipe configs that allows arbitrary feature combinations - Dict-based node features: Changed
SimSample.node_featuresfrom a single tensor[N, 4]to a dict withcoordsandfeatureskeys, allowing cleaner separation of coordinates from other node attributes - Updated stats handling: Renamed
thickness_statstofeature_statsto reflect the generic nature of the new feature system - Consistent updates across all models: Updated all rollout models (Transolver, MeshGraphNet) and training/inference code to handle the new dict structure
The refactoring enables more flexibility in defining input features for crash simulation models while maintaining backward compatibility through the config system.
Confidence Score: 4/5
- Safe to merge with minor cleanup recommended
- The refactoring is well-structured and consistently applied across all files. The dict-based feature structure is properly handled everywhere. Minor issues include outdated docstring and unused dead code that should be cleaned up but won't cause runtime errors.
- datapipe.py:90-97 (outdated docstring) and datapipe.py:337-341 (unused method)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/d3plot_reader.py | 5/5 | Added Reader class wrapper and changed key from mesh_pos to coords in returned dict |
| examples/structural_mechanics/crash/datapipe.py | 4/5 | Major refactor to support configurable features as dict structure; outdated docstring and unused _normalize_thickness_tensor method |
| examples/structural_mechanics/crash/rollout.py | 5/5 | Updated all rollout models to handle node_features as dict with coords and features keys |
| examples/structural_mechanics/crash/train.py | 5/5 | Added reader instantiation and updated stats dict key from thickness to feature |
Sequence Diagram
sequenceDiagram
participant Hydra
participant Trainer
participant Reader
participant Dataset
participant Model
Hydra->>Trainer: instantiate(cfg.reader)
Note over Hydra,Reader: Creates Reader with wall_node_disp_threshold
Hydra->>Trainer: instantiate(cfg.datapipe, reader=reader)
Trainer->>Dataset: __init__(reader, features=[thickness])
Dataset->>Reader: __call__(data_dir, num_samples, split)
Reader->>Reader: process_d3plot_data()
Reader-->>Dataset: (srcs, dsts, [{coords, thickness}])
Note over Dataset: Concatenate features into [N,F] tensor
Dataset->>Dataset: _compute_feature_stats()
Dataset->>Dataset: normalize features
Note over Dataset: Store as node_features_data
Trainer->>Model: train()
Model->>Dataset: __getitem__(idx)
Dataset->>Dataset: build_xy(idx)
Note over Dataset: Returns dict: {coords: [N,3], features: [N,F]}
Dataset-->>Model: SimSample(node_features={coords, features})
Model->>Model: forward(sample)
Note over Model: Extract coords and features from dict
Model->>Model: rollout with features
Additional Comments (2)
-
examples/structural_mechanics/crash/datapipe.py, line 90-97 (link)style: Docstring is outdated - references "thickness stats" but now uses generic "feature stats"
-
examples/structural_mechanics/crash/datapipe.py, line 337-341 (link)style: Unused method -
_normalize_thickness_tensoris never called after refactoring to generic features
7 files reviewed, 2 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Extended the crash example README with two major documentation sections explaining the datapipe and reader architecture. The additions clarify how raw simulation data flows through the reader into model-ready tensors, document the stable two-part input interface (x['coords'] and x['features']), explain feature ordering and normalization, and provide guidance for implementing custom readers via Hydra configuration.
Key improvements:
- Documented datapipe's responsibility for constructing inputs/targets and computing normalization statistics
- Explained that feature column order matches configuration order and is preserved deterministically
- Described reader contract: return
(srcs, dsts, point_data)with'coords'and per-feature arrays - Provided extensibility guidance with example YAML and Python signatures for custom readers
- Clarified "no features" case handling and forward compatibility patterns
Confidence Score: 5/5
- This PR is safe to merge with no risk
- This is a documentation-only change with no code modifications. The added documentation accurately describes the existing implementation, provides clear extensibility guidance, and improves developer experience without any potential for runtime issues or breaking changes
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/README.md | 5/5 | Added comprehensive documentation sections explaining datapipe input construction, feature ordering, normalization, and reader extensibility |
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant Reader as d3plot_reader.Reader
participant Dataset as CrashBaseDataset
participant Model
User->>Hydra: Configure reader & datapipe
Hydra->>Reader: Instantiate with config
Hydra->>Dataset: __init__(reader, features, ...)
Dataset->>Reader: __call__(data_dir, num_samples, split, logger)
Reader->>Reader: find_run_folders()
Reader->>Reader: load_d3plot_data()
Reader->>Reader: parse_k_file() for thickness
Reader->>Reader: compute_node_type() to filter walls
Reader->>Reader: build_edges_from_mesh_connectivity()
Reader-->>Dataset: (srcs, dsts, point_data)
Note over Dataset: point_data contains:<br/>{'coords': [T,N,3],<br/> 'thickness': [N]}
Dataset->>Dataset: Build mesh_pos_seq & node_features_data
Dataset->>Dataset: _compute_autoreg_node_stats()
Dataset->>Dataset: _compute_feature_stats()
Dataset->>Dataset: Normalize positions & features
Model->>Dataset: __getitem__(idx)
Dataset->>Dataset: build_xy(idx)
Dataset-->>Model: SimSample(x={'coords': [N,3],<br/>'features': [N,F]}, y=[N,(T-1)*3])
Note over Model: Model receives stable interface:<br/>x['coords'] + x['features']<br/>with deterministic column order
1 file reviewed, no comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Refactored the crash datapipe to support configurable node features through a flexible dict-based architecture. The key changes are:
- d3plot_reader.py: Added
Readerwrapper class for Hydra instantiation, changed output dict key frommesh_postocoordsfor consistency - datapipe.py: Refactored
SimSampleto storenode_featuresas a dict withcoordsand optional feature tensors (e.g.,thickness). Updated dataset classes to accept areadercallable andfeatureslist parameter, compute generic feature statistics instead of thickness-specific stats - rollout.py: Updated all 6 rollout model classes to extract
coordsandfeaturesfrom the dict-basednode_featuresstructure
The refactoring maintains backward compatibility through default values and enables extensibility for adding new node features beyond thickness.
Confidence Score: 4/5
- This PR is safe to merge with minimal risk after verification
- Score reflects a well-structured refactoring with consistent changes across all affected files. The dict-based architecture is properly implemented with defensive
.get()calls and default values. Deducted one point because this is a significant architectural change that warrants thorough integration testing to ensure the new feature extraction pipeline works correctly with various feature configurations. - examples/structural_mechanics/crash/datapipe.py - verify the feature extraction and normalization logic works correctly with different feature configurations
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/d3plot_reader.py | 5/5 | Added Reader class wrapper and changed dict key from 'mesh_pos' to 'coords' - clean abstraction layer |
| examples/structural_mechanics/crash/datapipe.py | 4/5 | Refactored to support configurable features as dict of tensors - major architectural change, well-structured but needs verification |
| examples/structural_mechanics/crash/rollout.py | 5/5 | Updated all rollout models to handle new dict-based node_features structure - consistent changes across all models |
Sequence Diagram
sequenceDiagram
participant Config as Hydra Config
participant Reader as d3plot_reader.Reader
participant Dataset as CrashBaseDataset
participant Sample as SimSample
participant Model as Rollout Models
Config->>Reader: Instantiate with wall_node_disp_threshold
Dataset->>Reader: Call reader(data_dir, num_samples, split, logger)
Reader->>Reader: process_d3plot_data()
Reader-->>Dataset: Return (srcs, dsts, point_data)
Note over Reader,Dataset: point_data contains {"coords": [T,N,3], "thickness": [N]}
Dataset->>Dataset: Extract coords from point_data["coords"]
Dataset->>Dataset: Extract features from point_data[feature_keys]
Dataset->>Dataset: Concatenate features into [N,F] tensor
Dataset->>Dataset: Compute feature statistics (_compute_feature_stats)
Dataset->>Dataset: Normalize coords and features
Model->>Dataset: __getitem__(idx)
Dataset->>Dataset: build_xy(idx)
Dataset->>Sample: Create SimSample with node_features dict
Note over Dataset,Sample: node_features = {"coords": [N,3], "features": [N,F]}
Sample-->>Model: Return SimSample
Model->>Sample: Extract coords = node_features["coords"]
Model->>Sample: Extract features = node_features.get("features", default)
Model->>Model: Concatenate with velocity/time for input
Model->>Model: Forward pass and prediction
3 files reviewed, 1 comment
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Extended the crash datapipe to support configurable readers via Hydra, enabling VTP file support alongside existing d3plot format.
Key Changes
- Refactored datapipe to accept generic
readercallable instead of hardcodedprocess_d3plot_data - Changed
SimSample.node_featuresfrom single tensor to dict with'coords'and'features'keys - Renamed
mesh_postocoordsandthickness_stats.jsontofeature_stats.jsonfor generality - Added
featureslist parameter to datapipe configs to specify which node-wise features to include - Created Reader classes for both d3plot and VTP formats with Hydra configs
- Added comprehensive documentation to README explaining datapipe/reader architecture
Critical Issues Found
- VTP reader has null pointer bugs:
loggeris used without null checks on lines 148, 168, 172, causingAttributeErrorwhen logger=None - Feature mismatch: Both datapipe configs specify
features: [thickness]but VTP reader only returnscoords, causingKeyErrorat runtime - Default config switched to VTP reader, but the feature configuration is incompatible with it
Confidence Score: 0/5
- This PR will cause immediate runtime failures and cannot run successfully
- Multiple critical bugs that guarantee runtime failure: (1) null pointer errors in vtp_reader.py when logger methods are called without null checks, (2) KeyError when datapipe tries to access 'thickness' feature that VTP reader doesn't provide. The default config uses VTP reader with thickness feature enabled, making this broken out-of-the-box.
- vtp_reader.py (null pointer bugs), conf/datapipe/graph.yaml and conf/datapipe/point_cloud.yaml (feature mismatch)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/vtp_reader.py | 1/5 | New VTP reader added but has critical bugs: missing logger null checks cause AttributeError on lines 148, 168, 172 |
| examples/structural_mechanics/crash/conf/datapipe/graph.yaml | 1/5 | Critical mismatch: features configured as [thickness] but VTP reader doesn't provide thickness, will cause KeyError |
| examples/structural_mechanics/crash/conf/datapipe/point_cloud.yaml | 1/5 | Critical mismatch: features configured as [thickness] but VTP reader doesn't provide thickness, will cause KeyError |
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant CrashBaseDataset
participant VTPReader
participant VTPFile
participant Datapipe
User->>Hydra: Load config.yaml with reader: vtp
Hydra->>Hydra: Instantiate vtp_reader.Reader
User->>CrashBaseDataset: Initialize dataset
CrashBaseDataset->>VTPReader: Call reader(data_dir, num_samples, split, logger)
VTPReader->>VTPFile: Read .vtp files from data_dir
VTPFile-->>VTPReader: Return poly.points + displacement_t* arrays
VTPReader->>VTPReader: Build edges from connectivity
VTPReader->>VTPReader: Stack positions [T,N,3]
VTPReader-->>CrashBaseDataset: Return (srcs, dsts, point_data)
Note over CrashBaseDataset: point_data contains {'coords': [T,N,3]}
CrashBaseDataset->>CrashBaseDataset: Extract features from point_data
Note over CrashBaseDataset: ERROR: KeyError if feature 'thickness' not in point_data
CrashBaseDataset->>CrashBaseDataset: Normalize coords and features
CrashBaseDataset->>Datapipe: Return SimSample with node_features dict
Datapipe-->>User: Training-ready tensors
4 files reviewed, 7 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR extends the crash datapipe with comprehensive documentation and refactoring to support pluggable readers via Hydra. The changes document the datapipe architecture, reader patterns, and feature extraction workflow while adding a VTP reader alongside the existing d3plot reader.
Major changes:
- Added extensive documentation to README.md explaining datapipe input/output contracts, reader interface, and configuration patterns
- Introduced pluggable reader architecture with
Readerclasses for both d3plot and VTP formats - Refactored datapipe to accept
readercallable andfeatureslist for flexible feature extraction - Changed config to default to VTP reader with
features: [thickness] - Added crash animation GIF to documentation
- Renamed stats file from
thickness_stats.jsonto genericfeature_stats.json
Critical Issue: The default config sets reader: vtp and features: [thickness], but the VTP reader only returns coords without thickness data. This will cause a KeyError at runtime when the datapipe tries to extract thickness from VTP records (datapipe.py:165).
Confidence Score: 2/5
- This PR has critical runtime errors that will prevent training with default VTP configuration
- Score reflects critical config mismatch where VTP reader lacks thickness data but configs request it, causing KeyError. Also multiple logger AttributeErrors in vtp_reader.py when logger is None
- examples/structural_mechanics/crash/vtp_reader.py (logger errors), examples/structural_mechanics/crash/conf/datapipe/graph.yaml and point_cloud.yaml (thickness feature mismatch)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/README.md | 4/5 | Adds comprehensive documentation for datapipe architecture and reader patterns; includes emojis in title and animation |
| docs/img/crash/crash_case4_reduced.gif | 5/5 | Binary image asset for documentation visualization |
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant Dataset as CrashBaseDataset
participant Reader as Reader (VTP/D3plot)
participant Datapipe as Datapipe Processing
participant Stats as Stats Manager
User->>Hydra: Load config (reader: vtp, features: [thickness])
Hydra->>Dataset: Instantiate with reader callable
Dataset->>Reader: __call__(data_dir, num_samples, split, logger)
Reader->>Reader: Load VTP/d3plot files
Reader->>Reader: Extract coords [T,N,3] and features
Reader-->>Dataset: Return (srcs, dsts, point_data)
Dataset->>Dataset: For each record in point_data
Dataset->>Dataset: Extract coords and features by name
Dataset->>Dataset: Concatenate features in config order
alt split == "train"
Dataset->>Stats: Compute node stats (pos_mean, pos_std)
Dataset->>Stats: Compute feature stats (feature_mean, feature_std)
Stats-->>Dataset: Save to stats/node_stats.json, feature_stats.json
else split != "train"
Dataset->>Stats: Load cached stats from JSON
Stats-->>Dataset: Return stats
end
Dataset->>Datapipe: Normalize trajectories with pos stats
Dataset->>Datapipe: Normalize features with feature stats
Dataset-->>User: Ready for training/inference
1 file reviewed, 6 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR extends the crash datapipe to support multiple reader formats (d3plot and VTP) and introduces a configurable features system. The README has been significantly expanded with quickstart instructions, datapipe architecture explanation, and reader documentation.
Major changes:
- New
vtp_reader.pyfor reading VTP/PolyData format crash simulations - Refactored
datapipe.pyto support configurable feature lists via Hydra config - Updated configs to use
features: [thickness]instead ofwall_node_disp_threshold - Comprehensive README expansion with quickstart, troubleshooting, and API documentation
- Added crash animation GIF to documentation
Critical issues found:
- VTP reader has logger-related bugs that will cause
NameErrorandAttributeErrorat runtime - Config mismatch:
features: [thickness]set but VTP reader doesn't provide thickness data, causingKeyError - These bugs block the VTP reader path entirely until fixed
Confidence Score: 1/5
- This PR introduces critical runtime errors that block the new VTP reader functionality
- Score of 1 reflects multiple logic errors in
vtp_reader.py(logger undefined/None causing NameError/AttributeError) and config mismatch (features: [thickness] but VTP provides no thickness) that will cause immediate crashes when using VTP reader path. The d3plot path may work, but the new feature is broken. - Pay close attention to
vtp_reader.py(logger bugs),conf/datapipe/graph.yamlandconf/datapipe/point_cloud.yaml(thickness feature mismatch)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/README.md | 4/5 | Major documentation expansion with quickstart, datapipe explanation, and reader guide. Emojis added to title without clear justification. |
| examples/structural_mechanics/crash/vtp_reader.py | 1/5 | New VTP reader with critical bugs: logger used without being passed to collect_mesh_pos, logger can be None causing AttributeError at lines 148, 168, 172. |
| examples/structural_mechanics/crash/datapipe.py | 3/5 | Refactored to support configurable features list. _feature_slices built but unused. Will crash if VTP reader used with features: [thickness] config. |
| examples/structural_mechanics/crash/conf/datapipe/graph.yaml | 1/5 | Changed to features: [thickness] which will cause KeyError when using VTP reader (VTP doesn't provide thickness data). |
| examples/structural_mechanics/crash/conf/datapipe/point_cloud.yaml | 1/5 | Changed to features: [thickness] which will cause KeyError when using VTP reader (VTP doesn't provide thickness data). |
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant CrashBaseDataset
participant Reader as Reader (VTP/D3plot)
participant Datapipe as Datapipe Logic
participant Model
User->>Hydra: Launch train.py with config
Hydra->>Hydra: Load config (reader, datapipe, features list)
Hydra->>CrashBaseDataset: Instantiate dataset with reader callable
CrashBaseDataset->>Reader: Call reader(data_dir, num_samples, split, logger)
alt D3plot Reader
Reader->>Reader: Parse d3plot files + .k files
Reader->>Reader: Extract coords, thickness, connectivity
Reader-->>CrashBaseDataset: Return (srcs, dsts, point_data with thickness)
else VTP Reader
Reader->>Reader: Parse .vtp files
Reader->>Reader: Extract coords, connectivity (NO thickness)
Reader-->>CrashBaseDataset: Return (srcs, dsts, point_data WITHOUT thickness)
end
CrashBaseDataset->>Datapipe: Loop over point_data records
Datapipe->>Datapipe: Check for 'coords' key
Datapipe->>Datapipe: Check for features[k] in rec
alt Feature exists
Datapipe->>Datapipe: Concatenate feature arrays
Datapipe->>Datapipe: Normalize features
else Feature missing (VTP + thickness config)
Datapipe->>Datapipe: Raise KeyError (CRASH)
end
CrashBaseDataset->>CrashBaseDataset: Compute stats, normalize trajectories
CrashBaseDataset-->>User: Dataset ready
User->>Model: Train with dataset
Model->>CrashBaseDataset: __getitem__(idx)
CrashBaseDataset->>Datapipe: build_xy(idx)
Datapipe-->>Model: Return x={'coords', 'features'}, y=positions
1 file reviewed, 5 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR extends the crash datapipe by adding comprehensive documentation, visualization assets (roof crash and crushcan animations), and a more modular architecture. The README was extensively rewritten to include a detailed quickstart guide, datapipe explanation, reader documentation, and troubleshooting section.
Key changes:
- Added two new GIF animations (
roof_crash.gif,crushcan.gif) for visual documentation - Comprehensive README rewrite with sections on datapipe architecture, reader patterns, and model I/O
- Documentation explains the new pluggable reader system and feature concatenation approach
- Added quickstart guide with configuration examples
- Included troubleshooting/FAQ section
Documentation quality:
- The new documentation is thorough and well-structured
- Clear explanation of how the datapipe transforms raw simulation data into model inputs
- Good coverage of the reader interface and how to implement custom readers
- Practical examples for configuration and usage
Note: The title includes emojis (🧱💥🚗) which may not align with repository documentation standards.
Confidence Score: 4/5
- This PR is safe to merge with minor considerations
- Score reflects that this is primarily a documentation change with visual assets. The changes are non-breaking and improve user experience. Minor deduction for the use of emojis in the title which may not align with documentation standards, but this is a style preference rather than a functional issue. The documentation itself is comprehensive and well-written.
- No files require special attention - primarily documentation and binary assets
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/README.md | 4/5 | Extensive documentation rewrite with new sections, quickstart guide, and troubleshooting; includes emojis in title |
Sequence Diagram
sequenceDiagram
participant User
participant Train
participant Dataset
participant Reader
participant D3plot/VTP
participant Model
User->>Train: python train.py
Train->>Dataset: Initialize CrashBaseDataset(reader, features)
Dataset->>Reader: __call__(data_dir, num_samples, split, logger)
Reader->>D3plot/VTP: Load simulation files
D3plot/VTP-->>Reader: Raw positions, connectivity, features
Reader-->>Dataset: (srcs, dsts, point_data)
Note over Dataset: point_data = [{'coords': [T,N,3], 'thickness': [N], ...}]
Dataset->>Dataset: Compute position statistics (mean, std)
Dataset->>Dataset: Compute feature statistics (per-column)
Dataset->>Dataset: Normalize coords and features
loop For each sample
Dataset->>Dataset: Build SimSample with dict features
Note over Dataset: node_features = {'coords': [N,3], 'features': [N,F]}
end
Train->>Model: Forward pass with x['coords'], x['features']
Model-->>Train: Predictions
Train->>Train: Compute loss and backprop
1 file reviewed, no comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR significantly expands the crash dynamics example documentation. It adds three visual examples (body-in-white, crushcan, roof crash) and comprehensive sections covering:
- Quickstart guide with step-by-step configuration instructions
- Detailed datapipe architecture explanation describing the input/output contract between readers, datapipe, and models
- Reader interface documentation for both built-in readers (d3plot and VTP) plus guidance for custom reader implementation
- Troubleshooting/FAQ section addressing common configuration issues
Key improvements:
- Clear separation of concerns between reader (data loading), datapipe (normalization/tensor construction), and model (inference)
- Explicit documentation of the
featuresconfiguration list and its role in determining feature tensor column ordering - Future-proofing guidance for custom reader signatures using
**kwargs
Documentation inconsistencies found:
- The quickstart example shows
features: [thickness]but the VTP reader only providescoordsand doesn't populate thickness data (see vtp_reader.py:197) - Multiple sections give conflicting examples for the VTP reader feature configuration
Confidence Score: 3/5
- Documentation-only change with inconsistencies between examples and actual code behavior
- This is a documentation-only PR that adds valuable explanatory content but contains logical inconsistencies. The quickstart examples show using
features: [thickness]with the VTP reader, but the VTP reader implementation only returnscoordsin its point_data records (vtp_reader.py:197). This will cause KeyError at runtime when users follow the documentation. The score reflects good documentation structure but incorrect guidance that would prevent successful usage. - examples/structural_mechanics/crash/README.md needs consistent examples throughout—either use
features: []everywhere for VTP or document how to extend the VTP reader to provide thickness
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/README.md | 4/5 | Comprehensive documentation expansion covering datapipe architecture, reader interfaces, quickstart guide, and usage examples. Adds animated GIFs for three crash scenarios and detailed troubleshooting section. |
Sequence Diagram
sequenceDiagram
participant User
participant Config as Hydra Config
participant Reader as Reader (d3plot/VTP)
participant Datapipe
participant Model as Model (Transolver/MGN)
participant Output as VTP Output
User->>Config: Select reader, datapipe, model recipe
User->>Config: Set raw_data_dir, num_time_steps, features list
Note over Config: training/default.yaml defines dataset paths
Note over Config: datapipe/*.yaml defines features order
User->>Reader: Launch train.py
Reader->>Reader: Load d3plot or VTP files from data_dir
Reader->>Reader: Extract coords [T,N,3] and feature arrays
Reader-->>Datapipe: Return (srcs, dsts, point_data)
Datapipe->>Datapipe: Construct x['coords'] and x['features']
Datapipe->>Datapipe: Compute normalization statistics
Datapipe->>Datapipe: Save stats to ./stats/
Datapipe-->>Model: Provide normalized inputs x and targets y
Model->>Model: Train with rollout scheme
Model-->>Output: Save trained checkpoint
User->>Model: Launch inference.py
Model->>Datapipe: Load test data with saved normalization stats
Datapipe-->>Model: Provide normalized test inputs
Model->>Model: Rollout predictions
Model->>Output: Write predicted VTP files
Output-->>User: View predictions in ParaView
1 file reviewed, 3 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR significantly expands the crash example documentation by adding:
- Quickstart guide with step-by-step configuration instructions
- Three new visualization examples (Body-in-White, Crushcan, Roof Crash modeling) with GIF animations
- Comprehensive "Datapipe" section explaining input construction, normalization, and the features interface
- Detailed "Reader" section documenting built-in d3plot and VTP readers plus guide for custom readers
- Performance tips and Troubleshooting/FAQ sections
The documentation improvements make the crash modeling pipeline much more accessible to new users by clearly explaining the modular architecture (reader → datapipe → model) and how components interact.
Issues found:
- Formatting typo on line 355 (
-.instead of-) - Documentation inconsistency: quickstart example shows
features: [thickness]but this conflicts with VTP reader capabilities documented later (VTP reader only provides coords, not thickness)
Confidence Score: 3/5
- Safe to merge after addressing the documentation inconsistency about VTP reader features
- The changes are documentation-only with no code modifications. There's a formatting typo and a significant documentation inconsistency where the quickstart misleads users about VTP reader capabilities (shows
features: [thickness]when VTP reader doesn't provide thickness). This inconsistency could cause runtime KeyErrors for users following the quickstart with VTP reader. - examples/structural_mechanics/crash/README.md requires clarification on feature configuration differences between d3plot and VTP readers
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/README.md | 3/5 | Major documentation improvements with quickstart guide and detailed datapipe/reader sections; contains formatting error and documentation inconsistency about VTP features |
Sequence Diagram
sequenceDiagram
participant User
participant Config as Hydra Config
participant Reader as Reader (d3plot/VTP)
participant Datapipe
participant Model as Model (Transolver/MGN)
User->>Config: Select reader, datapipe, model
Config->>Reader: Instantiate with features list
Reader->>Reader: Load raw simulation files
Reader->>Reader: Extract coords [T,N,3] & features
Reader->>Datapipe: Return (srcs, dsts, point_data)
Datapipe->>Datapipe: Concatenate features in config order
Datapipe->>Datapipe: Compute normalization stats
Datapipe->>Datapipe: Build x['coords'], x['features']
Datapipe->>Model: Provide x (dict), y (targets)
Model->>Model: Train/Inference
Model->>User: Output predictions as VTP files
1 file reviewed, 2 comments
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>
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>
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>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR extends the crash datapipe by adding a new VTP file reader as an alternative to the d3plot reader. The VTP reader loads .vtp files containing displacement fields and builds mesh connectivity from PolyData. Extensive documentation was added to the README covering datapipe internals, reader interfaces, and usage patterns.
Key changes:
- New
vtp_reader.pywithReaderclass that implements the reader interface - Reader config files for both d3plot and vtp formats
- Updated README with quickstart guide, datapipe documentation, and reader extension guide
- Default reader changed from d3plot to vtp in
conf/config.yaml - Datapipe configs updated to include
features: [thickness]
Critical issue: The datapipe configs specify features: [thickness] but the VTP reader only returns {"coords": mesh_pos_all} without thickness data. This creates a direct contradiction with the README documentation (line 255) which explicitly states VTP should use features: []. This will cause a KeyError at datapipe.py:165 when the datapipe tries to access the missing thickness feature during dataset construction.
Confidence Score: 2/5
- This PR has a critical configuration mismatch that will cause runtime errors
- Score reflects a blocking issue: datapipe configs request thickness feature that VTP reader doesn't provide, causing KeyError. The code quality is good (proper logger guards, **kwargs parameter), but the config-reader-documentation mismatch makes the default VTP configuration unusable
- Pay close attention to
conf/datapipe/point_cloud.yamlandconf/datapipe/graph.yaml- both need features changed to empty list to match VTP reader capabilities
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/vtp_reader.py | 4/5 | New VTP reader added with proper logger guards and **kwargs parameter; returns only coords (no thickness), which conflicts with datapipe configs |
| examples/structural_mechanics/crash/README.md | 4/5 | Extensive documentation added for VTP reader and datapipe; documents VTP should use features: [] but configs actually set features: [thickness] |
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant Datapipe
participant VTPReader
participant VTPFile
User->>Hydra: load config (reader: vtp, features: [thickness])
Hydra->>Datapipe: instantiate CrashPointCloudDataset
Datapipe->>VTPReader: __call__(data_dir, num_samples, split, logger)
VTPReader->>VTPReader: find_run_folders(data_dir)
loop For each .vtp file
VTPReader->>VTPFile: load_vtp_file(vtp_path)
VTPFile-->>VTPReader: pos_raw[T,N,3], mesh_connectivity
VTPReader->>VTPReader: build_edges_from_mesh_connectivity
VTPReader->>VTPReader: collect_mesh_pos (optional write VTP)
end
VTPReader-->>Datapipe: (srcs, dsts, point_data: [{"coords": ...}])
Datapipe->>Datapipe: validate "coords" key exists
Datapipe->>Datapipe: check for feature "thickness" in rec
Note over Datapipe: KeyError: "thickness" not in point_data!
Datapipe--xUser: Runtime error
2 files reviewed, 2 comments
mnabian
changed the title
Collaborator
Author
|
/blossom-ci |
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR extends the crash dynamics example with significant improvements to modularity and flexibility. The changes introduce a pluggable reader system supporting both d3plot and VTP formats, refactor the datapipe to use dictionary-based node features with configurable feature lists, and update all rollout implementations to work with the new structure. A comprehensive README provides quickstart instructions, detailed datapipe/reader documentation, and troubleshooting tips.
Key changes:
- Added new
vtp_reader.pywith proper logger guards (addresses previousAttributeErrorconcerns) - Refactored
datapipe.pyto accept reader callables and support flexible feature configuration viafeatures: [...]list - Updated
rollout.pyclasses to consume dictionary-based node features (coordsandfeatureskeys) - Added comprehensive test coverage in
tests/test_rollout.py - Extensive README documentation with architecture overview and usage examples
Issue identified:
- Config files
conf/datapipe/graph.yamlandconf/datapipe/point_cloud.yamlspecifyfeatures: [thickness], but the VTP reader only providescoordsand doesn't include thickness data. This will causeKeyErroratdatapipe.py:165when using VTP reader with these configs (as documented in README.md:255, VTP should usefeatures: []).
Confidence Score: 4/5
- Safe to merge after fixing config mismatch between VTP reader capabilities and datapipe feature requirements
- The implementation is well-designed with proper error handling and comprehensive tests. However, config files have a mismatch where
features: [thickness]is specified but VTP reader doesn't provide thickness, causing runtime KeyError. This is a known issue already flagged in previous comments. Once configs are aligned (either VTP reader extended or features set to []), this will be production-ready. - Config files
conf/datapipe/graph.yamlandconf/datapipe/point_cloud.yamlneedfeatures: []when using VTP reader, or VTP reader must be extended to provide thickness data
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/vtp_reader.py | 5/5 | New VTP reader implementation. Clean implementation with proper logger guards addressing previous concerns. |
| examples/structural_mechanics/crash/datapipe.py | 5/5 | Refactored to support pluggable readers and flexible feature configuration. Node features now dictionary-based with 'coords' and 'features' keys. |
| examples/structural_mechanics/crash/rollout.py | 5/5 | Updated to handle new dictionary-based node features structure from datapipe refactor. Properly accesses 'coords' and 'features' keys. |
| examples/structural_mechanics/crash/conf/datapipe/graph.yaml | 3/5 | Config still specifies features: [thickness] but VTP reader doesn't provide thickness, causing potential runtime errors. |
| examples/structural_mechanics/crash/conf/datapipe/point_cloud.yaml | 3/5 | Config still specifies features: [thickness] but VTP reader doesn't provide thickness, causing potential runtime errors. |
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant Reader
participant Datapipe
participant Rollout
participant Model
User->>Hydra: Load config (reader, datapipe, model)
Hydra->>Reader: Instantiate reader (d3plot or VTP)
Hydra->>Datapipe: Instantiate datapipe with reader
Datapipe->>Reader: __call__(data_dir, num_samples, split, logger)
Reader->>Reader: Parse files (d3plot or .vtp)
Reader->>Reader: Extract coords [T,N,3] and features
Reader-->>Datapipe: Return (srcs, dsts, point_data)
Datapipe->>Datapipe: Build node_features dict: {'coords': [N,3], 'features': [N,F]}
Datapipe->>Datapipe: Compute stats and normalize
Datapipe->>Datapipe: Create SimSample objects
User->>Rollout: forward(sample)
Rollout->>Rollout: Extract coords = sample.node_features['coords']
Rollout->>Rollout: Extract features = sample.node_features['features']
loop Autoregressive steps
Rollout->>Rollout: Compute velocity, normalize
Rollout->>Rollout: Build fx_t = cat([vel_norm, features, time])
Rollout->>Model: forward(fx=fx_t, embedding=positions)
Model-->>Rollout: Predicted acceleration/position
Rollout->>Rollout: Update positions
end
Rollout-->>User: Return trajectory [T,N,3]
16 files reviewed, no comments
Collaborator
Author
|
/blossom-ci |
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Added new VTP reader (vtp_reader.py) for crash simulation data preprocessing. The reader loads VTP files containing displacement time-series data, extracts mesh connectivity, builds edge graphs, and optionally writes processed VTP frames for visualization.
Key implementation details:
- Reads
.vtpfiles with displacement fields nameddisplacement_t* - Extracts mesh connectivity from PolyData faces
- Builds graph edges from mesh connectivity
- Returns
(srcs, dsts, point_data)tuple compatible with crash datapipe - Includes proper logger guards to prevent AttributeErrors when logger is None
- Supports
**kwargsfor future extensibility as documented in README - Correctly passes logger parameter through function call chain
All previously identified issues from review comments have been addressed in this revision.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- All previously identified critical bugs (AttributeErrors from logger usage, missing parameters, missing **kwargs) have been properly fixed. The code now has correct guard conditions, proper parameter passing, and follows documented patterns. No new issues introduced.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash/vtp_reader.py | 5/5 | New VTP reader implementation with proper logger guards, **kwargs support, and correct parameter passing - all previous review issues addressed |
Sequence Diagram
sequenceDiagram
participant User
participant Reader
participant process_vtp_data
participant find_run_folders
participant load_vtp_file
participant pyvista
participant collect_mesh_pos
User->>Reader: __call__(data_dir, num_samples, split, logger)
Reader->>Reader: Determine write_vtp based on split
Reader->>process_vtp_data: process_vtp_data(data_dir, num_samples, write_vtp, logger)
process_vtp_data->>find_run_folders: find_run_folders(data_dir)
find_run_folders->>find_run_folders: List .vtp files
find_run_folders->>find_run_folders: Sort naturally by filename
find_run_folders-->>process_vtp_data: List of VTP file paths
loop For each VTP file (up to num_samples)
process_vtp_data->>load_vtp_file: load_vtp_file(vtp_path)
load_vtp_file->>pyvista: Read VTP file
pyvista-->>load_vtp_file: PolyData with displacement fields
load_vtp_file->>load_vtp_file: Extract coords and displacement_t* fields
load_vtp_file->>load_vtp_file: Sort displacement fields naturally
load_vtp_file->>load_vtp_file: Compute positions (coords + displacements)
load_vtp_file->>load_vtp_file: Extract mesh connectivity from faces
load_vtp_file-->>process_vtp_data: pos_raw[T,N,3], mesh_connectivity
process_vtp_data->>process_vtp_data: Build edges from connectivity
process_vtp_data->>process_vtp_data: Extract src/dst node indices
process_vtp_data->>collect_mesh_pos: collect_mesh_pos(output_dir, pos_raw, connectivity, write_vtp, logger)
loop For each timestep
collect_mesh_pos->>collect_mesh_pos: Create PolyData mesh for timestep
collect_mesh_pos->>collect_mesh_pos: Add displacement field
alt write_vtp == True
collect_mesh_pos->>pyvista: Save VTP file
alt logger exists
collect_mesh_pos->>logger: Log saved filename
end
end
end
collect_mesh_pos-->>process_vtp_data: mesh_pos_all[T,N,3]
process_vtp_data->>process_vtp_data: Append point_data with coords
end
process_vtp_data-->>Reader: (srcs, dsts, point_data_all)
Reader-->>User: Tuple of edge lists and point data
1 file reviewed, no comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PhysicsNeMo Pull Request
Description
Checklist
Dependencies