Skip to content

fix: make StructuredGrid3DGeometry agree with the structured support it duplicates - #366

Merged
lachlangrose merged 1 commit into
test/scalar-field-grid-registrationfrom
refactor/repair-structured-grid-geometry
Nov 1, 2026
Merged

lachlangrose merged 1 commit into
test/scalar-field-grid-registrationfrom
refactor/repair-structured-grid-geometry

Conversation

@lachlangrose

Copy link
Copy Markdown
Member

Step 1 of consolidating the structured-grid geometry code. No behaviour change for the interpolation path.

Background

StructuredGrid3DGeometry is a copy of the geometry half of BaseStructuredSupport (24 of its ~25 members are shared). It has drifted. The only in-library user is the aabb_grid of the UnstructuredMesh*Geometry classes, and it had no tests. Before it can be used as the single geometry type (a later step), it has to agree with the support. This PR makes it agree and pins that with tests.

What was different

Found by running both classes on the same inputs (unit cube, offset anisotropic box, 30-degree rotated box):

Member Geometry class (before) Support (and now geometry)
global_node_indices stub, returned its input, so elements was (n, 8, 3) ijk triples flat node indexes, elements is (n, 8)
position_to_cell_index truncating, no clip, no outside handling: x=6 on a 4-cell grid gave cell 6 floor, clipped to the last cell, outside points get (0,0,0) and inside=False
inside strict > / <, boundary excluded, no tolerance, no input validation boundary included to a small tolerance, validates input
global_index_to_node_index 1-D only any shape
global_cell_indices missing added

global_index_to_cell_index also dropped a modulo on the k index. It gives the same result for valid indices.

Tests

packages/loop_common/tests/test_structured_grid_geometry.py (22 tests) compares the geometry class with loop_common.supports.StructuredGrid on three grids, and covers the maximum boundary, outside points, single points and bad shapes. 19 of the 22 fail on the old code; the other 3 were already correct.

  • packages/loop_common/tests: 178 passed
  • packages/loop_interpolation/tests: 606 passed, 46 skipped, 2 xfailed
  • tests: 730 passed, 6 skipped
  • ruff: no new findings

Notes

  • Behaviour change: inside() and position_to_cell_index on StructuredGrid3DGeometry now include boundary points. I found no callers outside the library besides the aabb_grid use, but I have not checked the QGIS plugin or map2loop.
  • Neither this class nor the support applies rotation_xy in position_to_cell_index or inside; points must already be in the grid's own frame. The docstring now says so.
  • Not in this PR: the 2D class (same pattern, same drift likely), the other support members (position_to_cell_corners, node_indexes_to_position, element_size, vtk), and making BaseStructuredSupport extend this class.

🤖 Generated with Claude Code

…it duplicates

StructuredGrid3DGeometry is a copy of the geometry half of BaseStructuredSupport and
had drifted:

- global_node_indices was a stub returning its input, so elements was (n, 8, 3) ijk
  triples instead of (n, 8) flat node indexes
- position_to_cell_index did not floor, clip or flag points outside the grid (a point
  past the maximum got an out-of-range cell index) and ran inside() before validating
- inside() excluded the boundary and had no tolerance
- global_index_to_node_index only handled 1-D input; global_cell_indices was missing

Bring those in line with the support and add tests that compare the two on the same
inputs (unit, offset/anisotropic and minimal grids), plus explicit boundary and
outside-point cases.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@lachlangrose
lachlangrose force-pushed the refactor/repair-structured-grid-geometry branch from ea4bed7 to 3bdbb08 Compare November 1, 2026 06:54
@lachlangrose
lachlangrose merged commit 722f50a into master Nov 1, 2026
37 checks passed
@lachlangrose
lachlangrose deleted the refactor/repair-structured-grid-geometry branch November 1, 2026 06:54
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.

1 participant