domino crash model arch and example #1191
Conversation
RishikeshRanade
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces a specialized DoMINO model variant for structural mechanics crash simulations (physicsnemo.models.domino_crash) alongside a comprehensive example in examples/structural_mechanics/crash_domino/. The implementation extends the existing aerodynamics DoMINO architecture to handle transient crash physics by adding support for time-dependent geometry deformation, implicit/explicit integration schemes, and element-based mesh processing. The changes include new datapipes for loading CAE datasets, geometry encoding modules for multi-scale STL/SDF processing, and solution calculators with distance-weighted neighborhood aggregation. A complete training/validation/testing pipeline is provided with distributed training support, mixed precision, and caching infrastructure. However, critical issues exist: the model contains runtime errors (variable name mismatches, undefined parameters), the datapipe has uninitialized variables, configuration files reference incorrect datasets (aerodynamics DrivAer instead of crash data), and the README is entirely aerodynamics-focused despite being in the crash_domino directory.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| physicsnemo/models/domino_crash/model.py | 1/5 | Adds core DoMINO crash model with transient support but contains critical variable name error (line 615) and typo (line 705) causing runtime failures |
| physicsnemo/datapipes/cae/domino_crash_datapipe.py | 2/5 | Implements comprehensive crash simulation datapipe with uninitialized variable bug in downsample_geometry and missing timesteps parameter in process_volume |
| examples/structural_mechanics/crash_domino/src/train.py | 4/5 | Training script with distributed support, mixed precision, and checkpointing; minor issues include duplicate nvmlInit calls and unused variables |
| examples/structural_mechanics/crash_domino/src/test.py | 2/5 | Inference pipeline for transient crash simulations with incorrect loop bounds (lines 166, 206) and potentially wrong L2 error calculation (line 738) |
| physicsnemo/models/domino_crash/solutions.py | 2/5 | Solution calculators with multiple division-by-zero risks in parameter normalization and distance-weighted aggregation (lines 44, 72, 228-237, 357-366) |
| physicsnemo/models/domino_crash/geometry_rep.py | 3/5 | Geometry encoding module with potential undefined variable h in cross-attention path and hardcoded batch_size=1assumption (line 469) |
| examples/structural_mechanics/crash_domino/src/conf/config.yaml | 3/5 | Configuration file with incorrect data processor kind='drivaer_aws' and paths referencing aerodynamics dataset instead of crash data |
| examples/structural_mechanics/crash_domino/src/utils.py | 2/5 | Utility functions with critical indentation error (line 329) causing syntax error and hardcoded field indices in metrics |
| examples/structural_mechanics/crash_domino/src/loss.py | 2/5 | Loss computation with duplicate/wildcard imports and unused parameters suggesting incomplete refactoring from aerodynamics example |
| examples/structural_mechanics/crash_domino/src/crash_datapipe.py | 2/5 | CrashDataset class with mismatched global parameter handling (expects 'stress' but gets CFD params) and broken main example |
| physicsnemo/models/domino_crash/mlps.py | 1/5 | MLP wrappers with critical layer count mismatch in AggregationModel (documented 5 layers, produces 6) |
| physicsnemo/models/domino_crash/encodings.py | 2/5 | Geometry encoding modules lacking validation for invalid geo_encoding_type and missing docstring parameter |
| examples/structural_mechanics/crash_domino/README.md | 1/5 | Documentation is entirely copied from aerodynamics DoMINO without adaptation; describes fluid dynamics instead of crash mechanics |
| physicsnemo/datapipes/cae/cae_dataset.py | 3/5 | Adds transient support to statistics computation with potential axis specification issues and questionable batch_n calculation |
| examples/structural_mechanics/crash_domino/src/conf/cached.yaml | 2/5 | Cached data config with incorrect paths pointing to 'drivaer_aws' aerodynamics dataset instead of crash data |
| examples/structural_mechanics/crash_domino/src/compute_statistics.py | 4/5 | Statistics computation script with minor issues (unused dataset_info dict, unnecessary barrier) but solid distributed logic |
| examples/structural_mechanics/crash_domino/src/process_data.py | 4/5 | Parallel data preprocessing script with unused variables suggesting incomplete refactoring but sound multiprocessing approach |
| examples/structural_mechanics/crash_domino/src/validate_cache.py | 3/5 | Cache validation utility with potential torch.allclose crashes on incompatible dtypes and incorrect docstring description |
| physicsnemo/utils/domino/utils.py | 4/5 | Adds well-documented repeat_array utility function with comprehensive examples and proper torch integration |
| physicsnemo/models/domino_crash/init.py | 5/5 | Standard module initialization file cleanly exposing DoMINO crash model class |
| examples/structural_mechanics/crash_domino/requirements.txt | 4/5 | Adds appropriate dependencies for crash simulation example with minor formatting issue (missing trailing newline) |
Confidence score: 1/5
- This PR has multiple critical bugs that will cause immediate runtime failures and cannot be merged without substantial fixes.
- Score is reduced due to: (1) syntax errors and undefined variables in core model code (model.py lines 615-619, 705; utils.py line 329; datapipe lines 366-392, 644-737), (2) configuration files pointing to wrong datasets (aerodynamics instead of crash data), (3) MLP layer count mismatch breaking model architecture, (4) division-by-zero risks throughout solution calculators, (5) completely incorrect README documentation describing aerodynamics instead of crash mechanics, and (6) numerous unused variables/parameters suggesting incomplete adaptation from the aerodynamics template.
- Requires immediate attention to: all files in physicsnemo/models/domino_crash/ (especially model.py, solutions.py, mlps.py), the datapipe (domino_crash_datapipe.py), all configuration files (config.yaml, cached.yaml), the README, and example utilities (utils.py, crash_datapipe.py, loss.py).
Sequence Diagram
sequenceDiagram
participant User
participant Hydra
participant DataProcessor
participant CrashDataset
participant DoMINODataPipe
participant CAEDataset
participant DoMINO
participant GeometryRep
participant SolutionCalc
User->>Hydra: main(cfg)
Hydra->>Hydra: Load config.yaml
alt Data Processing Mode
User->>DataProcessor: process_data.py
DataProcessor->>CrashDataset: __init__(input_dir)
loop For each file
DataProcessor->>CrashDataset: __getitem__(idx)
CrashDataset->>CrashDataset: Read VTP displacement data
CrashDataset->>CrashDataset: Extract time series info
CrashDataset->>CrashDataset: Calculate surface coordinates/normals
CrashDataset-->>DataProcessor: Return data dict
DataProcessor->>DataProcessor: Save as .npy
end
end
alt Training Mode
User->>User: train.py
User->>User: Initialize DistributedManager
User->>User: Load scaling factors
User->>DoMINO: __init__(model_parameters)
DoMINO->>GeometryRep: Create geo_rep_volume
DoMINO->>GeometryRep: Create geo_rep_surface
DoMINO->>SolutionCalc: Create solution_calculator_surf
DoMINO->>SolutionCalc: Create solution_calculator_vol
DoMINO-->>User: model
User->>CAEDataset: __init__(data_dir, keys_to_read)
CAEDataset->>CAEDataset: Infer file type (.npy/.zarr)
CAEDataset->>CAEDataset: Initialize BackendReader
CAEDataset-->>User: dataset
User->>DoMINODataPipe: __init__(config)
DoMINODataPipe->>DoMINODataPipe: set_dataset(dataset)
DoMINODataPipe-->>User: datapipe
loop Training Epochs
User->>DoMINODataPipe: __getitem__(idx)
DoMINODataPipe->>CAEDataset: __getitem__(idx)
CAEDataset->>CAEDataset: Read file (async if preloaded)
CAEDataset->>CAEDataset: Move to GPU
CAEDataset-->>DoMINODataPipe: raw data dict
DoMINODataPipe->>DoMINODataPipe: process_data()
DoMINODataPipe->>DoMINODataPipe: compute_stl_scaling_and_surface_grids()
DoMINODataPipe->>DoMINODataPipe: signed_distance_field(stl_vertices)
alt Surface Processing
DoMINODataPipe->>DoMINODataPipe: process_surface()
DoMINODataPipe->>DoMINODataPipe: downsample_geometry()
DoMINODataPipe->>DoMINODataPipe: knn(surface_coordinates)
DoMINODataPipe->>DoMINODataPipe: normalize(coordinates)
DoMINODataPipe->>DoMINODataPipe: scale_model_targets()
end
alt Volume Processing
DoMINODataPipe->>DoMINODataPipe: process_volume()
DoMINODataPipe->>DoMINODataPipe: sample_in_bbox filtering
DoMINODataPipe->>DoMINODataPipe: shuffle_array sampling
DoMINODataPipe->>DoMINODataPipe: signed_distance_field(volume_grid)
end
DoMINODataPipe-->>User: preprocessed batch
User->>DoMINO: forward(data_dict)
DoMINO->>GeometryRep: geo_rep_surface(stl_vertices)
GeometryRep->>GeometryRep: BQWarp ball query
GeometryRep->>GeometryRep: GeoConvOut projection
GeometryRep->>GeometryRep: UNet processing
GeometryRep-->>DoMINO: encoding_g_surf
alt Transient Model
loop Integration Steps
DoMINO->>DoMINO: surface_local_geo_encodings()
DoMINO->>SolutionCalc: solution_calculator_surf()
SolutionCalc->>SolutionCalc: apply_parameter_encoding()
SolutionCalc->>SolutionCalc: nn_basis(coordinates)
SolutionCalc->>SolutionCalc: aggregation_model()
SolutionCalc-->>DoMINO: displacement prediction
DoMINO->>DoMINO: Update coordinates += prediction
end
end
DoMINO-->>User: (volume_output, surface_output)
User->>User: compute_loss_dict()
User->>User: optimizer.step()
User->>User: save_checkpoint()
end
end
alt Testing Mode
User->>User: test.py
User->>User: Load checkpoint
loop For each test file
User->>User: Read VTP/STL files
User->>User: Calculate SDF
User->>User: Prepare data_dict
User->>DoMINO: forward(data_dict)
DoMINO-->>User: predictions
User->>User: Save as VTP with time series
User->>User: Create PVD collection
User->>User: Calculate L2 metrics
end
end
21 files reviewed, 65 comments
| tensorboard | ||
| cuml | ||
| einops | ||
| tensorstore No newline at end of file |
There was a problem hiding this comment.
style: missing newline at end of file
|
|
||
| val: # Validation configurable parameters | ||
| dataloader: | ||
| num_workers: 6 No newline at end of file |
There was a problem hiding this comment.
style: missing newline at end of file
| if transient: | ||
| axis = (0, 1) | ||
| else: | ||
| axis = (0) |
There was a problem hiding this comment.
syntax: axis should be a tuple when transient=False to maintain consistency
| if transient: | |
| axis = (0, 1) | |
| else: | |
| axis = (0) | |
| if transient: | |
| axis = (0, 1) | |
| else: | |
| axis = (0,) |
| batch_mean = field_data.mean(axis=axis) | ||
| batch_M2 = ((field_data - batch_mean) ** 2).sum(axis=axis) | ||
| batch_n = field_data.shape[0] |
There was a problem hiding this comment.
logic: batch_n is not calculated correctly for transient data - should account for both time and batch dimensions. Should batch_n be field_data.shape[0] * field_data.shape[1] when transient=True to count all samples across both dimensions?
| Args: | ||
| radii: The list of radii of the local geometry encodings. | ||
| neighbors_in_radius: The list of number of neighbors in the radius of the local geometry encodings. | ||
| geo_encoding_type: The type of geometry encoding to use. Can be "both", "stl", or "sdf". |
There was a problem hiding this comment.
style: n_upstream_radii parameter is not documented in the docstring
| pip install -r requirements.txt | ||
| ``` | ||
|
|
||
| ## Getting started with the DrivAerML example |
There was a problem hiding this comment.
logic: Section header mentions 'DrivAerML example' for external aerodynamics dataset, but this example is in crash_domino directory. Content misalignment with directory structure.
|
|
||
| ### Training the DoMINO model | ||
|
|
||
| To train and test the DoMINO model on AWS dataset, follow these steps: |
There was a problem hiding this comment.
logic: References 'AWS dataset' but no AWS dataset has been mentioned. Should this be 'DrivAerML' or a crash-specific dataset?
| [How-to Curate data for DoMINO Model](https://github.com/NVIDIA/physicsnemo-curator/blob/main/examples/external_aerodynamics/domino/README.md). | ||
|
|
||
| Caching is implemented in | ||
| [`CachedDoMINODataset`](https://github.com/NVIDIA/physicsnemo/blob/main/physicsnemo/datapipes/cae/domino_datapipe.py#L1250). |
There was a problem hiding this comment.
logic: Link points to domino_datapipe.py but based on the file list, crash uses domino_crash_datapipe.py. Reference may be incorrect for this crash variant.
| ### Training with Physics Losses | ||
|
|
||
| DoMINO supports enforcing of PDE residuals as soft constraints. This can be used | ||
| to improve the model predictions' adherence to the governing laws of the problem | ||
| which include Continuity and Navier Stokes equations. | ||
|
|
||
| Note, if you wish to modify the PDEs used for DoMINO, please edit the | ||
| `compute_physics_loss` function from `train.py` appropriately. | ||
|
|
||
| #### Prerequisites for PDE residuals | ||
|
|
||
| The computation of Physics residuals is supported using the PhysicsNeMo-Sym | ||
| library. Install it using | ||
|
|
||
| ```bash | ||
| pip install "Cython" | ||
| pip install "nvidia-physicsnemo.sym>2.1.0" --no-build-isolation | ||
| ``` | ||
|
|
||
| To execute the training using physics losses, run the `train.py` with the | ||
| configuration below | ||
|
|
||
| ```bash | ||
| torchrun --nproc_per_node=<num-gpus> train.py \ | ||
| ++train.add_physics_loss=True ++model.num_neighbors_volume=8 | ||
| ``` | ||
|
|
||
| Note, the `num_neighbors_volume` is set to 8 to reduce the memory requirement. | ||
| Also, when the Physics losses are applied, it will automatically sample | ||
| `num_neighbors_volume // 2` additional points, for each point in | ||
| `num_neighbors_volume`. These are considered as "2-hop" neighbors, which are | ||
| required to compute the higher order gradients required for Navier-Stokes | ||
| equations. Hence, even if `num_neighbors_volume` is set to 8, for the fields, | ||
| it will sample `num_neighbors_volume (num_neighbors_volume // 2 ) + 1` (in this | ||
| case 40) total points. | ||
|
|
||
| The results of physics addition can be found below (using the DrivAerML | ||
| dataset). The results are computed on the design ID 419 and 439 from the | ||
| validation set and averaged. | ||
|
|
||
| We observe that, addition of physics losses improves the model | ||
| predictions' ability to respect the governing laws better. | ||
|
|
||
| <!-- markdownlint-disable --> | ||
| <table><thead> | ||
| <tr> | ||
| <th></th> | ||
| <th></th> | ||
| <th colspan="2">L2 Errors</th> | ||
| </tr></thead> | ||
| <tbody> | ||
| <tr> | ||
| <td>Type</td> | ||
| <td>Variable</td> | ||
| <td>Baseline (full dataset)</td> | ||
| <td>Baseline + Physics (full dataset)</td> | ||
| </tr> | ||
| <tr> | ||
| <td rowspan="5">Volume</td> | ||
| <td>p</td> | ||
| <td>0.15413</td> | ||
| <td>0.17203</td> | ||
| </tr> | ||
| <tr> | ||
| <td>U_x</td> | ||
| <td>0.15566</td> | ||
| <td>0.16397</td> | ||
| </tr> | ||
| <tr> | ||
| <td>U_y</td> | ||
| <td>0.32229</td> | ||
| <td>0.34383</td> | ||
| </tr> | ||
| <tr> | ||
| <td>U_z</td> | ||
| <td>0.31027</td> | ||
| <td>0.32450</td> | ||
| </tr> | ||
| <tr> | ||
| <td>nut</td> | ||
| <td>0.21049</td> | ||
| <td>0.21883</td> | ||
| </tr> | ||
| <tr> | ||
| <td rowspan="4">Surface</td> | ||
| <td>p</td> | ||
| <td>0.16003</td> | ||
| <td>0.14298</td> | ||
| </tr> | ||
| <tr> | ||
| <td>wss_x</td> | ||
| <td>0.21476</td> | ||
| <td>0.20519</td> | ||
| </tr> | ||
| <tr> | ||
| <td>wss_y</td> | ||
| <td>0.31697</td> | ||
| <td>0.30335</td> | ||
| </tr> | ||
| <tr> | ||
| <td>wss_z</td> | ||
| <td>0.35056</td> | ||
| <td>0.32095</td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
|
|
||
| <table><thead> | ||
| <tr> | ||
| <th></th> | ||
| <th colspan="2">Residual L2 Error (Computed w.r.t true Residuals)</th> | ||
| <th></th> | ||
| </tr></thead> | ||
| <tbody> | ||
| <tr> | ||
| <td>Variable</td> | ||
| <td>Baseline (full dataset)</td> | ||
| <td>Baseline + Physics (full dataset)</td> | ||
| <td>% Improvement</td> | ||
| </tr> | ||
| <tr> | ||
| <td>continuity</td> | ||
| <td>30.352072</td> | ||
| <td>2.11262</td> | ||
| <td>93.04%</td> | ||
| </tr> | ||
| <tr> | ||
| <td>momentum_x</td> | ||
| <td>19.109278</td> | ||
| <td>2.33800</td> | ||
| <td>87.77%</td> | ||
| </tr> | ||
| <tr> | ||
| <td>momentum_y</td> | ||
| <td>99.36662</td> | ||
| <td>3.18452</td> | ||
| <td>96.80%</td> | ||
| </tr> | ||
| <tr> | ||
| <td>momentum_z</td> | ||
| <td>45.73862</td> | ||
| <td>2.691725</td> | ||
| <td>94.11%</td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| <!-- markdownlint-enable --> |
There was a problem hiding this comment.
logic: Entire physics loss section discusses Navier-Stokes equations, Continuity, and aerodynamics metrics (drag, lift). For crash simulations, the governing equations would be structural mechanics PDEs, not fluid dynamics.
| This repository includes examples of **DoMINO** training on the DrivAerML dataset. | ||
| However, many use cases require training **DoMINO** on a **custom dataset**. | ||
| The steps below outline the process. | ||
|
|
||
| 1. Reorganize that dataset to have the same directory structure as DrivAerML. The | ||
| raw data directory should contain a sepearte directory for each simulation. | ||
| Each simulation directory needs to contain mainly 3 files, `stl`, `vtp` and `vtu`, | ||
| correspoinding to the geometry, surface and volume fields information. | ||
| Additional details such as boundary condition information, for example inlet velocity, | ||
| may be added in a separate `.csv` file, in case these vary from one case to the next. | ||
| 2. Modify the following parameters in `conf/config.yaml` | ||
| - `project.name`: Specify a name for your project. | ||
| - `expt`: This is the experiment tag. | ||
| - `data_processor.input_dir`: Input directory where the raw simulation dataset is stored. | ||
| - `data_processor.output_dir`: Output directory to save the processed dataset (`.npy`). | ||
| - `data_processor.num_processors`: Number of parallel processors for data processing. | ||
| - `variables.surface`: Variable names of surface fields and fields type (vector or scalar). | ||
| - `variables.volume`: Variable names of volume fields and fields type (vector or scalar). | ||
| - `data.input_dir`: Processed files used for training. | ||
| - `data.input_dir_val`: Processed files used for validation. | ||
| - `data.bounding_box`: Dimensions of computational domain where most prominent solution | ||
| field variations. Volume fields are modeled inside this bounding box. | ||
| - `data.bounding_box_surface`: Dimensions of bounding box enclosing the biggest geometry | ||
| in dataset. Surface fields are modeled inside this bounding box. | ||
| - `train.epochs`: Set the number of training epochs. | ||
| - `model.volume_points_sample`: Number of points to sample in the volume mesh per epoch | ||
| per batch. | ||
| Tune based on GPU memory. | ||
| - `model.surface_points_sample`: Number of points to sample on the surface mesh per epoch | ||
| per batch. | ||
| Tune based on GPU memory. | ||
| - `model.geom_points_sample`: Number of points to sample on STL mesh per epoch per batch. | ||
| Ensure point sampled is lesser than number of points on STL (for coarser STLs). | ||
| - `eval.test_path`: Path of directory of raw simulations files for testing and verification. | ||
| - `eval.save_path`: Path of directory where the AI predicted simulations files are saved. | ||
| - `eval.checkpoint_name`: Checkpoint name `outputs/{project.name}/models` to evaluate | ||
| model. | ||
| - `eval.scaling_param_path`: Scaling parameters populated in `outputs/{project.name}`. | ||
| 3. Before running `process_data.py` to process the data, be sure to modify `openfoam_datapipe.py`. | ||
| This is the entry point for the user to modify the datapipe for dataprocessing. | ||
| A couple of things that might need to be changed are non-dimensionalizing schemes | ||
| based on the order of your variables and the `DrivAerAwsPaths` class with the | ||
| internal directory structure of your dataset. | ||
| For example, here is the custom class written for a different dataset. | ||
|
|
||
| ```python | ||
| class DriveSimPaths: | ||
| # Specify the name of the STL in your dataset | ||
| @staticmethod | ||
| def geometry_path(car_dir: Path) -> Path: | ||
| return car_dir / "body.stl" | ||
| # Specify the name of the VTU and directory structure in your dataset | ||
| @staticmethod | ||
| def volume_path(car_dir: Path) -> Path: | ||
| return car_dir / "VTK/simpleFoam_steady_3000/internal.vtu" | ||
| # Specify the name of the VTP and directory structure in your dataset | ||
| @staticmethod | ||
| def surface_path(car_dir: Path) -> Path: | ||
| return car_dir / "VTK/simpleFoam_steady_3000/boundary/aero_suv.vtp" | ||
| ``` |
There was a problem hiding this comment.
logic: References to DrivAerML dataset, automotive aerodynamics, and OpenFOAM file structures (VTK/simpleFoam) suggest this content is for CFD, not CAE crash simulations which typically use different file formats and solvers.
|
This PR has a lot of repetition from the DoMINO aerodynamics example, especially in the model. Can it be consolidated? If the original model code is not able to do so, can it be updated to accommodate both examples? |
| class GeoProcessor(nn.Module): | ||
| """Geometry processing layer using CNNs""" | ||
|
|
||
| def __init__(self, input_filters: int, output_filters: int, model_parameters): | ||
| """ | ||
| Initialize the GeoProcessor network. | ||
|
|
||
| Args: | ||
| input_filters: Number of input channels | ||
| model_parameters: Configuration parameters for the model | ||
| """ | ||
| super().__init__() | ||
| base_filters = model_parameters.base_filters | ||
| self.conv1 = nn.Conv3d( | ||
| input_filters, base_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv2 = nn.Conv3d( | ||
| base_filters, 2 * base_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv3 = nn.Conv3d( | ||
| 2 * base_filters, 4 * base_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv3_1 = nn.Conv3d( | ||
| 4 * base_filters, 4 * base_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv4 = nn.Conv3d( | ||
| 4 * base_filters, 2 * base_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv5 = nn.Conv3d( | ||
| 4 * base_filters, base_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv6 = nn.Conv3d( | ||
| 2 * base_filters, input_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv7 = nn.Conv3d( | ||
| 2 * input_filters, input_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.conv8 = nn.Conv3d( | ||
| input_filters, output_filters, kernel_size=3, padding="same" | ||
| ) | ||
| self.avg_pool = torch.nn.AvgPool3d((2, 2, 2)) | ||
| self.max_pool = nn.MaxPool3d(2) | ||
| self.upsample = nn.Upsample(scale_factor=2, mode="nearest") | ||
| self.activation = get_activation(model_parameters.activation) | ||
|
|
There was a problem hiding this comment.
logic: base_filters attribute is not set on GeoProcessor class but is accessed in the diff at line 419 (h = geometry_rep.geo_processor.base_filters). This will fail if model_parameters doesn't store it. Is base_filters guaranteed to be present on model_parameters passed to GeoProcessor.__init__? The class doesn't store it as an instance attribute.
| This code defines a distributed pipeline for training the DoMINO model on | ||
| CFD datasets. It includes the computation of scaling factors, instantiating |
There was a problem hiding this comment.
syntax: Docstring mentions 'CFD datasets' but this is for structural mechanics/crash analysis (same issue as other files in this crash_domino example)
| surf_loss_scaling, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
style: Empty line with trailing whitespace - remove
| if loss_type == "rmse": | ||
| dims = (0, 1, 2) | ||
| else: | ||
| dims = (0, 1, 2) |
There was a problem hiding this comment.
style: dims is always set to (0, 1, 2) regardless of loss_type. Remove the redundant conditional:
| if loss_type == "rmse": | |
| dims = (0, 1, 2) | |
| else: | |
| dims = (0, 1, 2) | |
| dims = (0, 1, 2) |
| if loss_type == "rmse": | ||
| denom = torch.sum(mask * (target - torch.mean(target, dims)) ** 2.0, dims) | ||
| loss = torch.mean(num / denom) |
There was a problem hiding this comment.
logic: potential double-averaging: torch.sum reduces over dims, then torch.mean averages the result again. If dims covers all dimensions, num and denom become scalars and torch.mean is redundant. Verify tensor shapes match intent. What are the expected shapes of output and target tensors? If dims=(0,1,2) reduces to a scalar, torch.mean on line 56 would be redundant
|
|
||
| num_surf_vars = 0 | ||
| surface_variable_names = [] | ||
| if model_type == "surface" or model_type == "combined": |
There was a problem hiding this comment.
logic: dead code path: model_type == "combined" is no longer valid since the function signature restricts to Literal["surface"]
|
|
||
| def load_scaling_factors( | ||
| cfg: DictConfig, logger=None | ||
| ) -> tuple[torch.Tensor, torch.Tensor]: |
There was a problem hiding this comment.
syntax: return type mismatch: signature says tuple[torch.Tensor, torch.Tensor] but function returns only one tensor on line 313
| metrics = { | ||
| "l2_displacement_x": torch.mean(l2[:, 0]), | ||
| "l2_displacement_y": torch.mean(l2[:, 1]), | ||
| "l2_displacement_z": torch.mean(l2[:, 2]), | ||
| } |
There was a problem hiding this comment.
logic: metrics assume 3-component displacement vector (x,y,z) indexed at [:, 0], [:, 1], [:, 2], but after summing over dim=(1,2), l2 loses its feature dimension. shape mismatch will cause IndexError
| #### Data Preprocessing | ||
|
|
||
| `PhysicsNeMo` has a related project to help with data processing, called | ||
| [PhysicsNeMo-Curator](https://github.com/NVIDIA/physicsnemo-curator). | ||
| Using `PhysicsNeMo-Curator`, the data needed to train a DoMINO model can be setup easily. | ||
| Please refer to | ||
| [these instructions on getting started](https://github.com/NVIDIA/physicsnemo-curator?tab=readme-ov-file#what-is-physicsnemo-curator) | ||
| with `PhysicsNeMo-Curator`. | ||
|
|
There was a problem hiding this comment.
style: References PhysicsNeMo-Curator for preprocessing but doesn't provide crash-specific curator instructions. Users may need guidance on how curator handles time-series VTP data extraction
| ```python | ||
| class CrashSimPaths: | ||
| # Specify the name of the STL in your dataset | ||
| @staticmethod | ||
| def geometry_path(sim_dir: Path) -> Path: | ||
| return sim_dir / "geometry.stl" | ||
| # Specify the name of the VTP with time-series data | ||
| @staticmethod | ||
| def surface_path(sim_dir: Path) -> Path: | ||
| return sim_dir / "displacement_timeseries.vtp" | ||
| ``` |
There was a problem hiding this comment.
style: Example code shows CrashSimPaths class structure but this differs from the actual CrashDataset implementation in crash_datapipe.py which uses global_params_reference dict structure. Ensure example matches actual API
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes focus on cleanup and removing redundancy: in physicsnemo/datapipes/cae/domino_crash_datapipe.py, two redundant instance variable assignments (self.use_surface_area and self.use_surface_normals) were removed since the code already validates and uses self.config.use_surface_area and self.config.use_surface_normals throughout; in examples/structural_mechanics/crash_domino/src/train.py, a docstring was updated from 'CFD datasets' to 'Crash datasets' to match the structural mechanics domain, and an extraneous blank line was removed; in examples/structural_mechanics/crash_domino/src/process_data.py, a transient_scheme parameter was added to CrashDataset initialization to support time-stepping crash simulations; in examples/structural_mechanics/crash_domino/src/loss.py, a redundant conditional that set dims = (0, 1, 2) in both branches was simplified to a single assignment; in physicsnemo/models/domino_crash/model.py, the output_features_vol parameter was removed from the constructor to convert the model to surface-only operation for crash simulations, and UNet initialization indentation was fixed. These changes align the DoMINO architecture and crash example with structural mechanics requirements by removing CFD-specific volume processing and correcting domain references.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| physicsnemo/datapipes/cae/domino_crash_datapipe.py | 5/5 | Removed redundant instance variable assignments for surface area and normals flags |
| examples/structural_mechanics/crash_domino/src/train.py | 5/5 | Updated docstring from CFD to Crash datasets and removed extraneous blank line |
| examples/structural_mechanics/crash_domino/src/process_data.py | 4/5 | Added transient_scheme parameter to CrashDataset initialization for time-stepping support |
| examples/structural_mechanics/crash_domino/src/loss.py | 4/5 | Simplified redundant conditional by removing duplicate dims assignment |
| physicsnemo/models/domino_crash/model.py | 4/5 | Removed output_features_vol parameter to convert model to surface-only crash operation |
Confidence score: 4/5
- This PR is safe to merge with minor risks related to incomplete code in two files
- Score reflects mostly clean simplifications and domain adaptation, but two files (crash_datapipe.py and utils.py) show empty critical sections suggesting incomplete implementation or missing context in the review
- Pay close attention to examples/structural_mechanics/crash_domino/src/process_data.py and physicsnemo/models/domino_crash/model.py to verify transient_scheme parameter compatibility and that volume-related logic removal doesn't break existing functionality
7 files reviewed, 4 comments
|
|
||
| def load_scaling_factors( | ||
| cfg: DictConfig, logger=None | ||
| ) -> torch.Tensor: |
There was a problem hiding this comment.
logic: return type changed to single tensor but function still returns only surf_factors_tensor which can be None when model_type != "surface" (line 312). Should the function raise an error or always guarantee a tensor return when model_type is not surface?
| l2_num = (pred - target) ** 2 | ||
| l2_num = torch.sum(l2_num, dim=(1,2)) | ||
| l2_num = torch.sqrt(l2_num) | ||
|
|
||
| l2_denom = target**2 | ||
| l2_denom = torch.sum(l2_denom, dim=(1,2)) | ||
| l2_denom = torch.sqrt(l2_denom) | ||
|
|
||
| l2 = l2_num / l2_denom |
There was a problem hiding this comment.
logic: after summing over dim=(1,2) on line 355, l2_num shape is (batch_size, features). Division on line 362 broadcasts incorrectly if shapes don't match. Verify target/pred have shape (batch, timesteps, features) and sum dims are correct. What are the expected input tensor shapes for pred and target? Should the sum be over spatial/point dimensions or time+point dimensions?
| # For implicit scheme, we need to add the displacements from the previous timestep to the current position | ||
| if self.transient_scheme == "implicit": | ||
| surface_fields_new = [] | ||
| for i in range(surface_coordinates.shape[0]-1): | ||
| surface_fields_new.append(surface_coordinates[i+1] - surface_coordinates[i]) | ||
| surface_fields = np.asarray(surface_fields_new) | ||
|
|
||
| surface_coordinates = surface_coordinates[:-1] | ||
| surface_normals = surface_normals[:-1] | ||
| surface_sizes = surface_sizes[:-1] |
There was a problem hiding this comment.
logic: implicit scheme creates mismatched tensor shapes. surface_fields becomes length N-1 (line 135), but surface_coordinates/normals/sizes are also sliced to N-1 (lines137-139). For implicit, targets should be N-1 but inputs should still be N to predict the next step. Should inputs remain unsliced? For implicit schemes, should the input coordinates/normals/sizes remain at full length N (unsliced) while targets are N-1, or are both inputs and targets meant to be N-1?
| surface_coordinates_all = [] | ||
| surface_normals_all = [] | ||
| surface_sizes_all = [] | ||
| for i in range(surface_fields.shape[0]): |
There was a problem hiding this comment.
logic: loop starts at range(surface_fields.shape[0]) (0-based), but for explicit scheme (default), this creates N+1 coordinate frames (line 126 concatenates initial + N). Previous code started at range(1, ...). Does explicit scheme need all N displacement frames or only N-1? Does the explicit scheme require all N displacement timesteps, or should the loop still start at index 1 to align with the original N-1 frame logic?
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds DoMINO transient model architecture and crash simulation example for structural mechanics applications. The implementation adapts the DoMINO architecture to handle time-series displacement predictions for crash simulations.
Major Changes:
- Added
physicsnemo/models/domino_transient/module with refactored model architecture supporting explicit and implicit time integration schemes - Added
physicsnemo/datapipes/cae/domino_datapipe_transient.pyfor transient data processing - Added complete crash simulation example in
examples/structural_mechanics/crash_domino/with training, testing, and data processing scripts
Critical Issues Found:
crash_datapipe.py:83-transient_schemeparameter is used but never defined in__init__, will cause NameErrorgeometry_rep.py:419- Accesses undefinedbase_filtersattribute ongeo_processor, will cause AttributeError when cross_attention is enabledREADME.md- Entire documentation describes aerodynamics/CFD instead of crash mechanics, needs complete rewrite
Other Issues:
- Multiple files have duplicate imports and unused imports that should be cleaned up
- Config file comments reference DrivAer/AWS aerodynamics dataset instead of crash data
Confidence Score: 1/5
- This PR has critical runtime errors that will prevent execution
- Two critical bugs will cause immediate runtime failures: (1) undefined
transient_schemeparameter in CrashDataset will throw NameError when instantiated, (2) missingbase_filtersattribute will throw AttributeError if cross_attention is enabled. The README is completely wrong for this application domain. - Critical attention needed:
examples/structural_mechanics/crash_domino/src/crash_datapipe.py(NameError),physicsnemo/models/domino_transient/geometry_rep.py(AttributeError),examples/structural_mechanics/crash_domino/README.md(wrong documentation)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash_domino/src/crash_datapipe.py | 1/5 | Critical bug: transient_scheme used without being defined in __init__ parameters, will cause NameError at runtime |
| physicsnemo/models/domino_transient/geometry_rep.py | 2/5 | Accesses base_filters attribute on line 419 that doesn't exist on geo_processor, will cause AttributeError when cross_attention is enabled |
| examples/structural_mechanics/crash_domino/src/loss.py | 3/5 | Duplicate imports on lines 18 and 22, wildcard import shadows specific imports - cleanup needed but functionally works |
| examples/structural_mechanics/crash_domino/src/conf/config.yaml | 3/5 | Configuration for crash simulations - comments reference DrivAer/CFD but content is crash-specific with Displacement/Stress variables |
| examples/structural_mechanics/crash_domino/README.md | 0/5 | README completely misaligned - describes external aerodynamics, DrivAerML dataset, and Navier-Stokes equations instead of crash simulation mechanics |
Sequence Diagram
sequenceDiagram
participant User
participant CrashDataset
participant DoMINODataPipe
participant DoMINO Model
participant SolutionCalculator
User->>CrashDataset: Load VTP crash data
CrashDataset->>CrashDataset: Read displacement time series
CrashDataset->>CrashDataset: Extract surface mesh (coordinates, normals, areas)
CrashDataset->>CrashDataset: Compute coordinates at each timestep (initial + displacements)
alt Implicit Scheme
CrashDataset->>CrashDataset: Convert to incremental displacements (coord[i+1] - coord[i])
else Explicit Scheme
CrashDataset->>CrashDataset: Keep absolute displacements from initial position
end
CrashDataset->>DoMINODataPipe: Return {surface_mesh_centers, surface_fields, surface_normals, surface_areas, ...}
DoMINODataPipe->>DoMINODataPipe: Compute SDF grids for geometry
DoMINODataPipe->>DoMINODataPipe: Normalize coordinates and fields
DoMINODataPipe->>DoMINODataPipe: Sample surface neighbors (KNN)
DoMINODataPipe->>DoMINO Model: Batched data dict
DoMINO Model->>DoMINO Model: Compute global geometry encodings (GeometryRep)
DoMINO Model->>DoMINO Model: Compute local geometry encodings (MultiGeometryEncoding)
DoMINO Model->>DoMINO Model: Compute positional encodings (FourierMLP)
loop For each timestep
alt Implicit Integration
DoMINO Model->>DoMINO Model: Update coordinates with predicted displacement
DoMINO Model->>SolutionCalculator: (updated_coords, encodings, params)
else Explicit Integration
DoMINO Model->>SolutionCalculator: (coords[t], encodings[t], params)
end
SolutionCalculator->>SolutionCalculator: Apply parameter encoding
SolutionCalculator->>SolutionCalculator: Sample surface neighbors
SolutionCalculator->>SolutionCalculator: Compute basis functions (FourierMLP)
SolutionCalculator->>SolutionCalculator: Aggregate with 1/distance weighting
SolutionCalculator->>DoMINO Model: Predicted displacement at timestep t
end
DoMINO Model->>User: Return (volume_output, surface_output)
17 files reviewed, 1 comment
|
|
||
| self.device = device | ||
| self.model_type = model_type | ||
| self.transient_scheme = transient_scheme |
There was a problem hiding this comment.
logic: transient_scheme used but not defined in __init__ parameters
| self.transient_scheme = transient_scheme | |
| self.model_type = model_type |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds new test script for DoMINO crash model inference on structural mechanics datasets.
Key changes:
- Implements distributed testing pipeline for crash simulations with time-series displacement predictions
- Supports both explicit and implicit time integration schemes
- Loads STL geometry, computes SDF-based encodings, and processes time-series VTP data
- Evaluates model predictions and saves results in VTP/PVD format with error metrics
Issues identified:
- Critical logic errors in implicit scheme time integration (lines 168-180, 598-609) that incorrectly accumulate displacements
- Multiple existing issues flagged in previous comments (variable mismatches, unused parameters, docstring inaccuracies)
Confidence Score: 1/5
- This PR has critical logical errors in the implicit scheme implementation that will produce incorrect results
- Score reflects critical bugs in implicit time integration logic that incorrectly accumulate displacements and update neighbor positions. These errors will cause wrong predictions and invalid test results. The implicit scheme path (lines 168-180) uses undefined variables in first iteration and incorrectly broadcasts displacements. The reconstruction logic (lines 598-609) compounds errors by adding positions instead of displacements. Combined with numerous other issues from previous comments, this requires significant fixes before merge.
- examples/structural_mechanics/crash_domino/src/test.py requires immediate attention for implicit scheme logic errors
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| examples/structural_mechanics/crash_domino/src/test.py | 1/5 | New test script with critical logical errors in implicit scheme implementation for time integration and result reconstruction |
Sequence Diagram
sequenceDiagram
participant Main as main()
participant Model as DoMINO Model
participant DataPrep as Data Preparation
participant TestStep as test_step()
participant Output as File Output
Main->>Main: Load config & init distributed
Main->>Main: Load scaling factors
Main->>Model: Initialize DoMINO model
Main->>Model: Load checkpoint weights
loop For each test file
Main->>DataPrep: Read STL mesh
DataPrep->>DataPrep: Compute geometry (vertices, faces, centers)
DataPrep->>DataPrep: Calculate SDF on grid
DataPrep->>DataPrep: Load time-series displacement data
DataPrep->>DataPrep: Prepare coordinates for explicit/implicit scheme
DataPrep->>DataPrep: Compute KNN neighbors
DataPrep->>Main: Return data_dict
Main->>TestStep: Pass data_dict, model, config
TestStep->>Model: Generate global geometry encoding
loop For each point batch
TestStep->>Model: Generate local geometry encodings
TestStep->>Model: Generate position encodings
alt Implicit Scheme
loop For each timestep
TestStep->>TestStep: Update positions with previous prediction
TestStep->>Model: Compute displacement prediction
TestStep->>TestStep: Store prediction
end
else Explicit Scheme
loop For each timestep
TestStep->>Model: Compute displacement prediction
TestStep->>TestStep: Store prediction
end
end
end
TestStep->>TestStep: Unnormalize predictions
TestStep->>Main: Return prediction_surf
Main->>Main: Reconstruct absolute coordinates
Main->>Main: Compute error metrics (L2, max error)
Main->>Output: Save predicted VTP files
Main->>Output: Save ground truth VTP files
Main->>Output: Write PVD collection files
end
Main->>Main: Report average errors across all files
1 file reviewed, 2 comments
| if cfg.model.transient_scheme == "implicit": | ||
| for i in range(cfg.model.integration_steps): | ||
| if i == 0: | ||
| surface_mesh_centers_batch_i = surface_mesh_centers_batch[:, i] | ||
| surface_mesh_neighbors_batch_i = surface_mesh_neighbors_batch[:, i] | ||
| prediction_surf[:, i, start_idx:end_idx] = surface_mesh_centers_batch_i[:, :, :3] | ||
| else: | ||
| surface_mesh_centers_batch_i[:, :, :3] += tpredictions_batch | ||
| for j in range(surface_mesh_neighbors_batch_i.shape[2]): | ||
| surface_mesh_neighbors_batch_i[:, :, j, :3] += tpredictions_batch | ||
|
|
||
| prediction_surf[:, i, start_idx:end_idx] = tpredictions_batch | ||
|
|
There was a problem hiding this comment.
logic: implicit scheme logic has ordering issue: in first iteration (i=0), surface_mesh_centers_batch_i is assigned from input but tpredictions_batch is computed after storing to output. For i>0, code uses tpredictions_batch from previous iteration to update positions, but the logic appears to accumulate displacements incorrectly. The neighbor update loop (lines 176-177) also incorrectly broadcasts the displacement to all neighbors identically.
| if cfg.model.transient_scheme == "implicit": | ||
| for i in range(num_timesteps): | ||
| if i == 0: | ||
| d_prediction_surf = surface_coordinates_initial | ||
| else: | ||
| d_prediction_surf = prediction_surf[i-1, :, :] | ||
| prediction_surf[i, :, :] = prediction_surf[i, :, :] + d_prediction_surf | ||
| elif cfg.model.transient_scheme == "explicit": | ||
| for i in range(num_timesteps): | ||
| prediction_surf[i, :, :] += surface_coordinates_initial | ||
| else: | ||
| raise ValueError(f"Invalid transient scheme: {cfg.model.transient_scheme}") |
There was a problem hiding this comment.
logic: implicit scheme reconstruction accumulates incorrectly: for i=0, adds surface_coordinates_initial (the initial position) to the displacement prediction. For i>0, adds the previous reconstructed position (not displacement), effectively double-counting displacements. Should track accumulated displacement separately or use a different indexing scheme.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces the DoMINO transient model architecture for structural mechanics crash simulations. The implementation adds support for time-dependent displacement predictions using both explicit and implicit integration schemes.
Key Changes
- New
domino_transientmodel module withDoMINOclass supporting transient analysis - Transient datapipe (
domino_datapipe_transient.py) with explicit/implicit time integration support - Crash-specific dataset loader for structural mechanics VTP files
- Example implementation in
examples/structural_mechanics/crash_domino/
Critical Issues Found
- Missing function parameter:
process_volumein datapipe missingtimestepsparameter (line 1035) - will cause NameError for transient models - Array initialization bug:
crash_datapipe.pyline 72 creates scalar instead of index array - Implicit scheme tensor mismatch: Input/target shapes inconsistent in implicit integration (crash_datapipe.py lines 132-141)
- Autoregressive rollout error: Test script has incorrect position accumulation for implicit scheme (test.py lines 168-177)
- Metrics dimension error:
metrics_fn_surfaceattempts to index 1D tensor as 2D (utils.py lines 404-418)
Recommendations
- Fix the missing
timestepsparameter inprocess_volumebefore merge to prevent runtime failures - Verify implicit scheme logic - current implementation has shape mismatches that will break training
- Add integration tests for both explicit and implicit schemes with known displacement sequences
- Consider adding validation checks for tensor shapes in time-stepping logic
Confidence Score: 2/5
- This PR has critical bugs that will cause runtime failures in transient crash simulations
- Score reflects multiple critical issues: (1) missing function parameter causing NameError, (2) implicit scheme has shape mismatches breaking training, (3) autoregressive test logic incorrect, (4) metrics computation will crash with IndexError. These are not edge cases but core functionality bugs that will prevent the transient model from working correctly.
- Critical attention needed:
physicsnemo/datapipes/cae/domino_datapipe_transient.py(missing parameter),examples/structural_mechanics/crash_domino/src/crash_datapipe.py(implicit scheme bugs),examples/structural_mechanics/crash_domino/src/test.py(rollout logic),examples/structural_mechanics/crash_domino/src/utils.py(metrics bug)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| physicsnemo/models/domino_transient/model.py | 4/5 | New transient DoMINO model architecture with explicit/implicit integration schemes - well-structured with helper methods but lacks validation for edge cases |
| physicsnemo/datapipes/cae/domino_datapipe_transient.py | 2/5 | Transient data processing pipeline - critical bug with missing timesteps parameter in process_volume will cause runtime failures |
| examples/structural_mechanics/crash_domino/src/crash_datapipe.py | 2/5 | Crash dataset loader - multiple bugs including incorrect array initialization and implicit scheme tensor shape mismatches that will break training |
| examples/structural_mechanics/crash_domino/src/test.py | 2/5 | Testing script with implicit scheme rollout - incorrect autoregressive accumulation logic will produce wrong predictions |
| examples/structural_mechanics/crash_domino/src/utils.py | 2/5 | Utility functions - critical tensor shape bug in metrics_fn_surface will cause IndexError at runtime |
Sequence Diagram
sequenceDiagram
participant User
participant CrashDataset
participant DoMINODataPipe
participant DoMINO Model
participant SolutionCalculator
User->>CrashDataset: Load VTP file with timesteps
CrashDataset->>CrashDataset: Extract displacement time series
CrashDataset->>CrashDataset: Compute initial coordinates + displacements
alt Implicit Scheme
CrashDataset->>CrashDataset: Convert to position deltas (N-1 timesteps)
else Explicit Scheme
CrashDataset->>CrashDataset: Keep absolute displacements (N timesteps)
end
CrashDataset-->>User: Return data dict
User->>DoMINODataPipe: Process data dict
DoMINODataPipe->>DoMINODataPipe: Normalize timesteps
DoMINODataPipe->>DoMINODataPipe: Sample time points (explicit/implicit)
DoMINODataPipe->>DoMINODataPipe: Compute geometry encodings (SDF, grids)
DoMINODataPipe->>DoMINODataPipe: Process surface mesh (KNN neighbors)
DoMINODataPipe->>DoMINODataPipe: Apply coordinate normalization
DoMINODataPipe-->>User: Preprocessed batch
User->>DoMINO Model: Forward pass
DoMINO Model->>DoMINO Model: Compute geometry representations
DoMINO Model->>DoMINO Model: Compute positional encodings
DoMINO Model->>DoMINO Model: Extract global parameters
alt Implicit Integration
loop For each integration step
DoMINO Model->>SolutionCalculator: Predict displacement delta
DoMINO Model->>DoMINO Model: Update positions with delta
DoMINO Model->>DoMINO Model: Update neighbor positions
end
else Explicit Integration
loop For each timestep
DoMINO Model->>SolutionCalculator: Predict displacement
end
end
SolutionCalculator->>SolutionCalculator: Sample neighbors in sphere
SolutionCalculator->>SolutionCalculator: Apply basis functions
SolutionCalculator->>SolutionCalculator: Aggregate with distance weighting
SolutionCalculator-->>DoMINO Model: Predicted displacements
DoMINO Model-->>User: (volume_output, surface_output)
7 files reviewed, 5 comments
| stl_vertices=data_dict["stl_coordinates"], | ||
| stl_indices=mesh_indices_flattened, | ||
| volume_fields=volume_fields_raw, | ||
| timesteps=timesteps, |
There was a problem hiding this comment.
logic: Missing timesteps parameter in process_volume signature - function references timesteps on lines 716-755 but parameter is not declared. Will cause NameError at runtime for transient models.
| timesteps=timesteps, | |
| def process_volume( | |
| self, | |
| c_min: torch.Tensor, | |
| c_max: torch.Tensor, | |
| volume_coordinates: torch.Tensor, | |
| volume_grid: torch.Tensor, | |
| center_of_mass: torch.Tensor, | |
| stl_vertices: torch.Tensor, | |
| stl_indices: torch.Tensor, | |
| volume_fields: torch.Tensor | None, | |
| volume_features: torch.Tensor | None, | |
| timesteps: torch.Tensor | None, | |
| ) -> dict[str, torch.Tensor]: |
|
|
||
| self.filenames = get_filenames(self.data_path) | ||
| random.shuffle(self.filenames) | ||
| self.indices = np.array(len(self.filenames)) |
There was a problem hiding this comment.
syntax: np.array(len(self.filenames)) creates a scalar array (just the length number), not an array of indices
| self.indices = np.array(len(self.filenames)) | |
| self.indices = np.arange(len(self.filenames)) |
| # For implicit scheme, we need to add the displacements from the previous timestep to the current position | ||
| if self.transient_scheme == "implicit": | ||
| surface_fields_new = [] | ||
| for i in range(surface_coordinates.shape[0]-1): | ||
| surface_fields_new.append(surface_coordinates[i+1] - surface_coordinates[i]) | ||
| surface_fields = np.asarray(surface_fields_new) | ||
|
|
||
| surface_coordinates = surface_coordinates[:-1] | ||
| surface_normals = surface_normals[:-1] | ||
| surface_sizes = surface_sizes[:-1] |
There was a problem hiding this comment.
logic: implicit scheme tensor slicing creates mismatched shapes. surface_fields becomes length N-1 after computing deltas (lines 134-137), but then surface_coordinates/normals/sizes are all sliced to [:-1] making them N-1 as well. For implicit time integration, inputs should remain at full length N to predict the next step, while targets should be N-1. This mismatch will cause dimension errors during training.
Does implicit scheme require inputs at full timestep count N (unsliced coordinates/normals/sizes) while targets are N-1 deltas?
| if cfg.model.transient_scheme == "implicit": | ||
| for i in range(cfg.model.integration_steps): | ||
| if i == 0: | ||
| surface_mesh_centers_batch_i = surface_mesh_centers_batch[:, i].clone() | ||
| surface_mesh_neighbors_batch_i = surface_mesh_neighbors_batch[:, i].clone() | ||
| else: | ||
| surface_mesh_centers_batch_i[:, :, :3] += tpredictions_batch | ||
| for j in range(surface_mesh_neighbors_batch_i.shape[2]): | ||
| surface_mesh_neighbors_batch_i[:, :, j, :3] += tpredictions_batch | ||
|
|
There was a problem hiding this comment.
logic: implicit scheme accumulation is incorrect. In first iteration (i=0), code assigns surface_mesh_centers_batch_i from input, then computes displacement. For i>0, adds displacement to previous positions, but the neighbor update loop (lines 176-177) broadcasts the same displacement to all neighbors identically, which doesn't account for individual neighbor displacements. This will cause incorrect position updates in autoregressive rollout.
| l2_num = (pred - target) ** 2 | ||
| l2_num = torch.sum(l2_num, dim=(1,2)) | ||
| l2_num = torch.sqrt(l2_num) | ||
|
|
||
| l2_denom = target**2 | ||
| l2_denom = torch.sum(l2_denom, dim=(1,2)) | ||
| l2_denom = torch.sqrt(l2_denom) | ||
|
|
||
| l2 = l2_num / l2_denom | ||
|
|
||
| metrics = { | ||
| "l2_displacement_x": torch.mean(l2[:, 0]), | ||
| "l2_displacement_y": torch.mean(l2[:, 1]), | ||
| "l2_displacement_z": torch.mean(l2[:, 2]), | ||
| } |
There was a problem hiding this comment.
logic: tensor shape mismatch will cause IndexError. After summing over dim=(1,2) on line 405, l2 shape is (batch_size,) with no feature dimension remaining. Lines 415-417 try to index l2[:, 0], l2[:, 1], l2[:, 2] which will fail since l2 is 1D.
What are the expected input shapes for pred and target? Should the sum be over spatial/point dimensions only, preserving the feature dimension?
2 participants
PhysicsNeMo Pull Request
Description
Checklist
Dependencies