Refactor functional module layout into category packages#1520
Refactor functional module layout into category packages#1520loliverhennigh wants to merge 18 commits intoNVIDIA:mainfrom
Conversation
|
/blossom-ci |
Greptile SummaryThis PR reorganizes Key issues found:
Important Files Changed
Last reviewed commit: "Merge branch 'main' ..." |
| from .mesh_poisson_disk_sample import MeshPoissonDiskSample, mesh_poisson_disk_sample | ||
| from .mesh_to_voxel_fraction import MeshToVoxelFraction, mesh_to_voxel_fraction | ||
| from .sdf import SignedDistanceField, signed_distance_field | ||
|
|
There was a problem hiding this comment.
Missing module files cause ImportError
geometry/__init__.py imports from .mesh_poisson_disk_sample and .mesh_to_voxel_fraction, but neither file exists in the physicsnemo/nn/functional/geometry/ directory. Only sdf.py was moved there. A full directory listing confirms only __init__.py and sdf.py are present.
This means any import physicsnemo.nn.functional (or any import that chains through physicsnemo/nn/functional/__init__.py, which imports from .geometry) will raise an ImportError at startup, breaking the entire nn.functional namespace.
The corresponding top-level __init__.py (line 19–20) also re-exports mesh_poisson_disk_sample and mesh_to_voxel_fraction, so those imports will fail too.
These two source files either need to be added to the geometry/ directory in this PR, or the geometry/__init__.py and functional/__init__.py should be updated to only reference sdf.py until the remaining files are migrated.
| from .interpolation import ( | ||
| grid_to_point_interpolation, | ||
| interpolation, | ||
| point_to_grid_interpolation, | ||
| ) |
There was a problem hiding this comment.
grid_to_point_interpolation and point_to_grid_interpolation don't exist in the interpolation package
physicsnemo/nn/functional/interpolation/__init__.py only exports Interpolation and interpolation — it has no grid_to_point_interpolation or point_to_grid_interpolation. Importing these names here will cause an ImportError at load time.
# current interpolation/__init__.py only exports:
from .interpolation import Interpolation, interpolation
__all__ = ["Interpolation", "interpolation"]Either:
- Add
grid_to_point_interpolationandpoint_to_grid_interpolationtophysicsnemo/nn/functional/interpolation/__init__.py(and create the backing modules), or - Revert these two names from this
__init__.pyuntil they are implemented.
physicsnemo/compat/__init__.py
Outdated
| "physicsnemo.models.layers.fully_connected_layers": "physicsnemo.nn.module.fully_connected_layers", | ||
| "physicsnemo.models.layers.fused_silu": "physicsnemo.nn.module.fused_silu", | ||
| "physicsnemo.models.layers.interpolation": "physicsnemo.nn.functional.interpolation", | ||
| "physicsnemo.models.layers.interpolation": "physicsnemo.nn.functional.interpolation.grid_to_point_interpolation", |
There was a problem hiding this comment.
Compat mapping points to a non-existent module
The compat target "physicsnemo.nn.functional.interpolation.grid_to_point_interpolation" does not correspond to any real module (there is no grid_to_point_interpolation.py file under physicsnemo/nn/functional/interpolation/). The install() function will catch the resulting ImportError and silently emit a warning instead of registering the alias, which means all legacy users of physicsnemo.models.layers.interpolation will see a deprecation warning but the compat shim will never be installed.
Once the backing module exists this should resolve itself, but it should be tracked alongside the fix for the missing grid_to_point_interpolation and point_to_grid_interpolation noted above.
| dynamic_output = ( | ||
| len(output) > 0 | ||
| and output[0].ndim == 2 | ||
| and output[0].shape[0] == 2 | ||
| and output[0].dtype in (torch.int32, torch.int64) | ||
| ) |
There was a problem hiding this comment.
Fragile heuristic for detecting dynamic vs. static output
dynamic_output is set to True when output[0].shape[0] == 2 and its dtype is integer, under the assumption that the unlimited-neighbors case returns a (2, total_count) index tensor. However, the limited/static case returns (N_queries, max_points) indices. If N_queries == 2, that tensor also has shape[0] == 2 and will incorrectly be classified as dynamic.
This causes the subsequent comparison branches to apply the wrong aggregation (sum(dim=0) instead of sum(dim=1)), potentially allowing a correctness difference between backends to go undetected when there are exactly two query points.
Consider disambiguating on a flag passed to compare_forward (e.g. whether max_points was None), or by inspecting the tensor's ndim and shape[1] more carefully against a known value of max_points.
|
|
||
| @classmethod | ||
| def make_inputs(cls, device: torch.device | str = "cpu"): | ||
| def make_inputs_forward(cls, device: torch.device | str = "cpu"): |
There was a problem hiding this comment.
The changes for function_spec.py that match this will come in the next PR. For now could you let this run through if that's alright.
There was a problem hiding this comment.
Some feedback, but overall looks reasonable to me. This is technically a semver-violating change (in that it breaks external imports of files which were not previously _-prefixed, and hence might break end-user code upon a minor version bump). If the engineering motivation for this is strong, though, we can do it. Let's at-minimum add a CHANGELOG.md note for this, and if there's anything we can do to signpost this for end-users to reduce the pain, that would be great.
I'll let Corey review the minor changes to domain_parallel and friends.
physicsnemo/nn/functional/neighbors/radius_search/radius_search.py
Outdated
Show resolved
Hide resolved
|
/blossom-ci |
PhysicsNeMo Pull Request
Description
Breaking up this PR,
#1457
This is just module layout a bit