-
Notifications
You must be signed in to change notification settings - Fork 534
Geotransolver Model #1297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Geotransolver Model #1297
Conversation
Greptile Summary
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (42)
-
examples/cfd/external_aerodynamics/transformer_models/src/conf/data/surface.yaml, line 23 (link)syntax: Typo: 'Surface-speficic' should be 'Surface-specific'
-
examples/cfd/external_aerodynamics/transformer_models/src/conf/transolver_volume.yaml, line 24 (link)style: Run ID references 'bfloat16' but precision is set to 'float32' on line 27. Should the run_id match the actual precision setting?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
examples/cfd/external_aerodynamics/transformer_models/src/conf/data/volume.yaml, line 23 (link)syntax: Typo: 'speficic' should be 'specific'
-
examples/cfd/external_aerodynamics/transformer_models/src/conf/data/volume.yaml, line 29 (link)style: Missing newline at end of file
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
examples/cfd/external_aerodynamics/transformer_models/src/conf/transolver_surface.yaml, line 24 (link)style: Run ID contains 'bfloat16' but precision is set to float32 on line 27 - consider updating for consistency
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
examples/cfd/external_aerodynamics/transformer_models/src/conf/transolver_surface.yaml, line 37 (link)style: Missing newline at end of file
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
test/models/geotransolver/test_context_projector.py, line 60 (link)style: Consider adding CPU testing for plus mode as well - limiting to CUDA-only may miss CPU-specific issues. Is there a specific reason plus mode only needs CUDA testing?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
examples/cfd/external_aerodynamics/transformer_models/src/compute_normalizations.py, line 92-94 (link)logic: Bug in Welford's algorithm:
Nis incremented twice (lines 77 and 92), causing incorrect variance calculation. Remove line 92 sinceNis already updated on line 77. -
examples/cfd/external_aerodynamics/transformer_models/src/compute_normalizations.py, line 77 (link)style: Variable
nis assigned but never used. Consider removing this line sincebatch_non line82serves the same purpose.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
test/models/geotransolver/test_geotransolver.py, line 192 (link)logic: Incorrect tensor indexing -
outputsis a single tensor, not a tuple -
test/models/geotransolver/test_geotransolver.py, line 411 (link)logic: Incorrect tensor indexing -
outputsis a single tensor, not a tuple -
test/models/geotransolver/test_geotransolver.py, line 460 (link)logic: Non-deterministic randomness breaks test reproducibility
-
test/models/geotransolver/test_geotransolver.py, line 740 (link)logic: Incorrect tensor indexing -
outputsis a single tensor, not a tuple -
examples/cfd/external_aerodynamics/transformer_models/src/metrics.py, line 172 (link)logic: Inconsistent tensor dimensionality -
mae_numis 3D but indexed as 2D here -
examples/cfd/external_aerodynamics/transformer_models/README.md, line 46 (link)style: The API reference mentions
physicsnemo.experimental.models.typhonbut the model is described as 'Typhon' throughout the document while the API suggests 'geotransolver'. Consider consistency in naming. Should the documentation consistently use 'Typhon' or 'GeoTransolver' as the model name to match the API structure?Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
examples/cfd/external_aerodynamics/transformer_models/README.md, line 34 (link)logic: The configuration examples mention
typhon_surfaceandtyphon_volumebut based on the file structure, these might actually begeotransolver_surfaceandgeotransolver_volume. Do the configuration names match the actual config files in the codebase? -
examples/cfd/external_aerodynamics/transformer_models/src/inference_on_vtk.py, line 662 (link)logic: Potential IndexError if no matching files found
-
examples/cfd/external_aerodynamics/transformer_models/src/inference_on_vtk.py, line 677 (link)logic: Potential IndexError if no matching files found
-
examples/cfd/external_aerodynamics/transformer_models/deprecated/inference_on_vtp.py, line 73-74 (link)logic: Bug:
surface_normalsis undefined. Should benormalson line 73. -
examples/cfd/external_aerodynamics/transformer_models/src/inference_on_zarr.py, line 543 (link)logic: Potential directory creation issue - path may not exist before writing file
-
examples/cfd/external_aerodynamics/transformer_models/src/inference_on_zarr.py, line 543 (link)logic: Datetime formatting could create invalid filenames due to colons and spaces
-
examples/cfd/external_aerodynamics/transformer_models/src/inference_on_zarr.py, line 568 (link)logic: Same directory and datetime filename issues as surface mode
-
examples/cfd/external_aerodynamics/transformer_models/src/utils.py, line 41-48 (link)style: Missing required docstring sections per MOD-003d. Add Parameters and Returns sections following NumPy style.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/context_projector.py, line 39 (link)logic: Class should inherit from physicsnemo.Module instead of nn.Module to follow MOD-001 coding standard
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/context_projector.py, line 313-314 (link)style: Remove commented debug print statements before merging
-
physicsnemo/experimental/models/geotransolver/context_projector.py, line 389 (link)logic: Argument order inconsistency - spatial_coords and geometry order differs from line 398. Should the argument order be consistent between extract_context_features and extract_local_features method calls?
-
physicsnemo/experimental/models/geotransolver/context_projector.py, line 126-128 (link)style: Missing jaxtyping tensor shape annotations in method signature per MOD-006
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/geotransolver.py, line 93-95 (link)syntax: Parameter type documentation is inconsistent. Docstring says 'int' but type annotation shows 'int | tuple[int, ...]'
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/geotransolver.py, line 128 (link)syntax: Default value documented as 512 but line 221 shows 32
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/geotransolver.py, line 329-381 (link)logic: Forward method missing tensor shape validation logic as required by MOD-005. Should validate shapes of local_embedding, geometry, and global_embedding tensors with torch.compiler.is_compiling() guard
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/geotransolver.py, line 331-335 (link)syntax: Missing jaxtyping tensor annotations for forward method parameters as required by MOD-006
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
examples/cfd/external_aerodynamics/transformer_models/deprecated/datapipe.py, line 64-78 (link)style: Function missing required docstring sections per MOD-003d. Should have Parameters and Returns sections.
Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/datapipes/cae/transolver_datapipe.py, line 509 (link)logic: Potential KeyError if outputs_surf doesn't contain 'fx' key when air_density/stream_velocity are missing. Should there be a check to ensure both outputs_surf and outputs_vol have the same keys before accessing fx?
-
physicsnemo/datapipes/cae/transolver_datapipe.py, line 568-569 (link)logic: The ValueError message doesn't handle the 'combined' model type case in the auto factor selection
-
examples/cfd/external_aerodynamics/transformer_models/src/train.py, line 21-25 (link)syntax: Duplicate import -
Sequenceis imported from bothtyping(line 21) andcollections.abc(line 25). Remove one of these imports. -
examples/cfd/external_aerodynamics/transformer_models/src/train.py, line 320 (link)logic: Potential bug:
loss.item()called on tensor that may be a list/multi-dimensional. This will fail if loss is not a scalar. Islossguaranteed to be a scalar tensor here, or could it be multi-dimensional when handling multiple point clouds? -
physicsnemo/experimental/models/geotransolver/gale.py, line 82 (link)syntax: Parameter
dimmissing type annotation. Should bedim: intper MOD-006 for jaxtyping tensor annotations in public function signatures.Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/gale.py, line 105-107 (link)logic: Method signature doesn't match docstring. Docstring describes
slice_tokensas single tensor but method expects list. Line 125 callstorch.cat(slice_tokens, dim=-2)which will fail ifslice_tokensis a single tensor. -
physicsnemo/experimental/models/geotransolver/gale.py, line 149-151 (link)syntax: Return type annotation is incorrect. Method returns
outputswhich is a list (line 211-213), not a single torch.Tensor as annotated. -
physicsnemo/experimental/models/geotransolver/gale.py, line 156-158 (link)syntax: Docstring parameter description doesn't match signature. Docstring describes
xas single tensor but signature expectstuple[torch.Tensor, ...].Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source) -
physicsnemo/experimental/models/geotransolver/gale.py, line 315 (link)syntax: Return type annotation is incorrect. Method returns
fxwhich is a list (line 338), not a single torch.Tensor as annotated. -
physicsnemo/experimental/models/geotransolver/gale.py, line 320-322 (link)syntax: Docstring parameter descriptions don't match method signature. Both
fxandglobal_contextare described as single tensors but signature expectstuple[torch.Tensor, ...]for both.Context Used: File from
greptile.json- CODING_STANDARDS/MODELS_IMPLEMENTATION.md (source)
40 files reviewed, 42 comments
|
|
||
| # Run batched inference using imported function from inference_on_zarr | ||
| with torch.no_grad(): | ||
| _, _, (predictions, _) = batched_inference_loop( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a shape mismatch here in the global_parameters setting. Global parameters have an extra dimension (1, 1, 2) in this case. We need to modify the datapipe to squeeze the first dimension.
PhysicsNeMo Pull Request
This PR brings GeoTransolver to physicsnemo and unifies the training recipe with Transolver.
Description
Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.