Skip to content

Refactor functional module layout into category packages#1520

Open
loliverhennigh wants to merge 18 commits intoNVIDIA:mainfrom
loliverhennigh:pr1-functional-layout
Open

Refactor functional module layout into category packages#1520
loliverhennigh wants to merge 18 commits intoNVIDIA:mainfrom
loliverhennigh:pr1-functional-layout

Conversation

@loliverhennigh
Copy link
Collaborator

PhysicsNeMo Pull Request

Description

Breaking up this PR,
#1457

This is just module layout a bit

@loliverhennigh
Copy link
Collaborator Author

/blossom-ci

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 19, 2026

Greptile Summary

This PR reorganizes physicsnemo/nn/functional by grouping related modules into category subpackages (fourier_spectral/, geometry/, neighbors/, regularization_parameterization/), and also splits benchmark helper methods into separate make_inputs_forward/make_inputs_backward variants with shared _BENCHMARK_CASES constants. The refactor is well-structured in principle, but the PR is incomplete in its current state — several source files referenced by the new package __init__.py files were not moved/created, causing ImportError at load time.

Key issues found:

  • Critical – geometry/__init__.py references non-existent modules: mesh_poisson_disk_sample.py and mesh_to_voxel_fraction.py are imported but do not exist in physicsnemo/nn/functional/geometry/. This breaks the entire physicsnemo.nn.functional namespace on import.
  • Critical – functional/__init__.py imports missing names: grid_to_point_interpolation and point_to_grid_interpolation are imported from .interpolation, but the interpolation package's __init__.py only exports Interpolation and interpolation. These will also raise ImportError.
  • Warning – compat map broken: The updated entry "physicsnemo.models.layers.interpolation" → "physicsnemo.nn.functional.interpolation.grid_to_point_interpolation" points to a module that doesn't exist. The compat shim will silently skip installing this alias and only emit a deprecation warning, leaving legacy users without a redirect.
  • Logic – fragile dynamic-output detection in RadiusSearch.compare_forward: The heuristic output[0].shape[0] == 2 to distinguish unlimited from limited radius-search output false-positives when N_queries == 2, potentially masking correctness differences between backends.

Important Files Changed

Filename Overview
physicsnemo/nn/functional/geometry/init.py New package init for the geometry category. Critically broken: imports MeshPoissonDiskSample, mesh_poisson_disk_sample, MeshToVoxelFraction, and mesh_to_voxel_fraction from modules that do not exist in the directory, causing an ImportError at package load time.
physicsnemo/nn/functional/init.py Top-level nn.functional init restructured to import from the new category subpackages. Broken by two issues: (1) mesh_poisson_disk_sample/mesh_to_voxel_fraction from .geometry are missing, and (2) grid_to_point_interpolation/point_to_grid_interpolation don't exist in interpolation/__init__.py, both causing ImportError.
physicsnemo/compat/init.py Updated two COMPAT_MAP entries: sdf correctly points to geometry.sdf, but interpolation now maps to a non-existent module interpolation.grid_to_point_interpolation, causing the compat shim to silently fail with a warning instead of installing the alias.
physicsnemo/nn/functional/neighbors/radius_search/radius_search.py Renamed and enhanced: added _BENCHMARK_CASES, split make_inputs into make_inputs_forward/make_inputs_backward, and implemented compare_forward/compare_backward. The docstring change (unused indices set to 0 instead of -1) aligns with the actual warp impl. compare_forward has a fragile dynamic_output heuristic that false-positives when N_queries == 2.
physicsnemo/nn/functional/neighbors/knn/knn.py Renamed into neighbors/knn/ subpackage. Clean changes: added _BENCHMARK_CASES, split make_inputs into make_inputs_forward, and implemented compare_forward (replacing the raise NotImplementedError). Looks correct.
physicsnemo/nn/functional/fourier_spectral/fft.py Renamed into fourier_spectral/ subpackage. All make_inputs methods split into make_inputs_forward and make_inputs_backward. Shared _FFT_1D_CASES/_FFT_2D_CASES constants extracted. Clean refactor with no functional changes.
physicsnemo/domain_parallel/shard_utils/knn.py Import path updated from physicsnemo.nn.functional.knn._cuml_impl to physicsnemo.nn.functional.neighbors.knn._cuml_impl. Correct path update reflecting the new package structure.
physicsnemo/domain_parallel/shard_utils/point_cloud_ops.py Import path updated from physicsnemo.nn.functional.radius_search._warp_impl to physicsnemo.nn.functional.neighbors.radius_search._warp_impl. Correct path update.

Last reviewed commit: "Merge branch 'main' ..."

Comment on lines +17 to +20
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

Copy link
Contributor

Choose a reason for hiding this comment

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

P0 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.

Comment on lines +23 to +27
from .interpolation import (
grid_to_point_interpolation,
interpolation,
point_to_grid_interpolation,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

P0 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_interpolation and point_to_grid_interpolation to physicsnemo/nn/functional/interpolation/__init__.py (and create the backing modules), or
  • Revert these two names from this __init__.py until they are implemented.

"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 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.

Comment on lines +169 to +174
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)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 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"):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@peterdsharpe peterdsharpe left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@coreyjadams coreyjadams left a comment

Choose a reason for hiding this comment

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

LGTM!

@loliverhennigh
Copy link
Collaborator Author

/blossom-ci

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants