-
Notifications
You must be signed in to change notification settings - Fork 70
feat(geometry): add GeometryArray for efficient repeated geometries #3215
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: develop
Are you sure you want to change the base?
feat(geometry): add GeometryArray for efficient repeated geometries #3215
Conversation
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.
3 files reviewed, 2 comments
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
Note: I haven't reviewed tests myself yet, so please skip reviewing tests for now |
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/geometry/base.pyLines 135-143 135 _ = np.linalg.inv(transform)
136 except np.linalg.LinAlgError as err:
137 if index is not None:
138 raise ValidationError(f"Transform at index {index} is not invertible.") from err
! 139 raise ValidationError("Transform matrix is not invertible.") from err
140
141
142 class Geometry(Tidy3dBaseModel, ABC):
143 """Abstract base class, defines where something exists in space."""Lines 3843-3851 3843 return np.matmul(translations, transforms)
3844
3845 def _get_full_transform(self, index: int) -> MatrixReal4x4:
3846 """Get the full 4x4 transform for a geometry at given index (transform + translation)."""
! 3847 return self._all_transforms[index]
3848
3849 @cached_property
3850 def _transformed_geometries(self) -> list[Transformed]:
3851 """List of transformed geometries in the array."""Lines 3900-3908 3900 List of 2D shapes that intersect plane.
3901 For more details refer to
3902 `Shapely's Documentation <https://shapely.readthedocs.io/en/stable/project.html>`_.
3903 """
! 3904 return self._geometry_group.intersections_tilted_plane(
3905 normal, origin, to_2D, cleanup=cleanup, quad_segs=quad_segs
3906 )
3907
3908 def intersections_plane(Lines 3954-3962 3954 -------
3955 bool
3956 Whether this geometry intersects the plane.
3957 """
! 3958 return self._geometry_group.intersects_axis_position(axis, position)
3959
3960 def inside(self, x: NDArray[float], y: NDArray[float], z: NDArray[float]) -> NDArray[bool]:
3961 """For input arrays ``x``, ``y``, ``z`` of arbitrary but identical shape, return an array
3962 with the same shape which is ``True`` for every point in zip(x, y, z) that is inside theLines 3985-3994 3985 def _surface_area(self, bounds: Bound) -> float:
3986 """Returns object's surface area within given bounds."""
3987 # Surface area cannot be reliably computed when non-trivial transforms are present
3988 if self.transforms is not None:
! 3989 log.warning("Surface area of transformed elements cannot be calculated.")
! 3990 return None
3991 # For pure translations, sum surface areas using local bounds for base geometry
3992 total_area = 0.0
3993 for geom in self._transformed_geometries:
3994 # Transform bounds to local coordinate systemLines 3995-4003 3995 vertices = np.dot(geom.inverse, Transformed._vertices_from_bounds(bounds))[:3]
3996 local_bounds = (tuple(vertices.min(axis=1)), tuple(vertices.max(axis=1)))
3997 instance_area = self.geometry.surface_area(local_bounds)
3998 if instance_area is None:
! 3999 return None
4000 total_area += instance_area
4001 return total_area
4002 tidy3d/components/geometry/utils.pyLines 247-255 247 flatten_array=flatten_array,
248 transform=new_transform,
249 )
250 elif flatten_array and isinstance(geometry, base.GeometryArray):
! 251 yield from flatten_groups(
252 *geometry._transformed_geometries,
253 flatten_nonunion_type=flatten_nonunion_type,
254 flatten_transformed=flatten_transformed,
255 flatten_array=flatten_array,Lines 282-290 282 elif isinstance(geometry, base.ClipOperation):
283 yield from traverse_geometries(geometry.geometry_a)
284 yield from traverse_geometries(geometry.geometry_b)
285 elif isinstance(geometry, base.GeometryArray):
! 286 yield from traverse_geometries(geometry.geometry)
287 yield geometry
288
289
290 def from_shapely(Lines 415-425 415 validate_no_transformed_polyslabs(geometry.geometry_a, transform)
416 validate_no_transformed_polyslabs(geometry.geometry_b, transform)
417 elif isinstance(geometry, base.GeometryArray):
418 # For GeometryArray, check each instance's transform combined with the base geometry
! 419 for i in range(geometry.num_geometries):
! 420 instance_transform = np.dot(transform, geometry._get_full_transform(i))
! 421 validate_no_transformed_polyslabs(geometry.geometry, instance_transform)
422
423
424 class SnapLocation(Enum):
425 """Describes different methods for defining the snapping locations.""" |
f2905bf to
c50e72c
Compare
…tances Add GeometryArray class that enables efficient representation of repeated geometric patterns using arrays of offsets and optional transformations.
c50e72c to
a7bfa3d
Compare
|
We‘ll need to consider adjoint support for this too @groberts-flex |
|
Great, thanks for the heads up! I'll take a look through and add this to the roadmap. |
| # translation matrix: [[1,0,0,x], [0,1,0,y], [0,0,1,z], [0,0,0,1]] | ||
| translations = np.broadcast_to(Transformed.identity(), shape).copy() | ||
| if self.offsets is not None: | ||
| translations[:, :3, 3] = self.offsets |
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.
We would just ignore the offset from self.transforms? Or do we want to forbid them?
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.
good catch. it's a bug here
Note
Medium Risk
Adds a new core geometry primitive and integrates it into serialization and geometry traversal/flattening paths; mistakes could affect simulation setup or geometry processing for users adopting the new type.
Overview
Introduces
GeometryArray, a newGeometrytype for representing many copies of a base geometry using per-instanceoffsetsand optional 4x4transforms, plus a convenience constructorGeometry.array(offsets=..., transforms=...).Wires the new type through public exports (
tidy3d.__init__), geometry utilities (flatten_groups/traverse_geometries/validate_no_transformed_polyslabs), and JSON schemas so simulations/components can serialize/deserializeGeometryArray. Adds docs and comprehensive tests for validation, bounds/inside behavior, plane intersections, volume/surface area behavior, and equivalence toGeometryGroup, along with shared validators for finite-geometry and invertible transforms.Written by Cursor Bugbot for commit a7bfa3d. This will update automatically on new commits. Configure here.
Greptile Overview
Greptile Summary
This PR introduces
GeometryArray, a new class for efficiently representing repeated geometry instances with optional transformations and translations. The implementation includes a convenience methodgeometry.array()available on all geometry objects.Key Changes
GeometryArrayclass: Stores a single base geometry with arrays of offsets and/or transformation matrices, creating transformed instances on-demand via cached propertiesassert_geometry_finite()andcheck_transform_invertible()validators to avoid code duplication betweenTransformedandGeometryArrayGeometryArraysupport inflatten_groups(),traverse_geometries(), andvalidate_no_transformed_polyslabs()GeometryGroupImplementation Details
The class efficiently handles three modes:
The implementation internally creates
Transformedgeometries (cached) and delegates most operations to them, ensuring consistency with existing geometry behavior.Confidence Score: 4/5
tidy3d/components/geometry/base.pyfor the docstring syntax correctionsImportant Files Changed
Sequence Diagram
sequenceDiagram participant User participant Geometry participant GeometryArray participant Transformed participant Structure User->>Geometry: Create base geometry (e.g., Box) User->>Geometry: Call geometry.array(offsets, transforms) Geometry->>GeometryArray: Create GeometryArray instance GeometryArray->>GeometryArray: Validate offsets and transforms GeometryArray->>GeometryArray: Check transforms are invertible GeometryArray->>GeometryArray: Validate geometry has finite bounds GeometryArray-->>User: Return GeometryArray instance User->>Structure: Create Structure with GeometryArray Structure->>GeometryArray: Query bounds GeometryArray->>GeometryArray: Compute _get_full_transform() for each instance GeometryArray->>Transformed: Create Transformed geometries (cached) Transformed-->>GeometryArray: Return transformed instances GeometryArray->>GeometryArray: Compute combined bounds GeometryArray-->>Structure: Return bounds User->>GeometryArray: Call inside(x, y, z) GeometryArray->>Transformed: Check inside() for each instance Transformed-->>GeometryArray: Return boolean arrays GeometryArray->>GeometryArray: OR results together GeometryArray-->>User: Return combined result