Skip to content

Conversation

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Nov 24, 2025

Cleaning up only helps in a few limited cases, e.g. mentioned here #2596. Even in this case, it is fine to classify that as a corner.

@dbochkov-flexcompute I'm not sure if this edge case matters to gap refinement or the convexity analysis for minimal feature size.

Greptile Overview

Greptile Summary

This PR disables polygon cleanup in the corner finder and adds performance optimizations to the meshing system.

Main Changes:

  • Disables polygon cleanup (SHAPELY_CLEANUP = False) in corner detection to preserve all geometric features, addressing edge cases where cleanup was too aggressive
  • Adds cleanup and quad_segs parameters throughout the geometry intersection API, allowing fine-grained control over polygon processing and circular shape discretization
  • Uses low-resolution discretization (N_SHAPELY_QUAD_SEGS = 8) for meshing operations versus high-resolution (200) for visualization
  • Optimizes mesher performance by caching min steps per interval (O(1) lookup vs recomputation), replacing O(n*m) operations with O(n log m) binary search using bisect and np.searchsorted, and vectorizing containment checks

Impact:
The cleanup disabling means corner detection will now classify more vertices as corners, which may affect mesh refinement. The PR description mentions this is fine for corner classification, but questions remain about impact on gap refinement and convexity analysis for minimal feature size. The performance optimizations in mesher.py should significantly improve meshing speed for complex geometries.

Confidence Score: 4/5

  • This PR is safe to merge with moderate risk pending clarification on edge cases
  • Score reflects well-structured API changes with proper parameter threading and comprehensive test coverage, but the PR author explicitly questions whether disabling cleanup affects gap refinement and convexity analysis, suggesting uncertainty about full impact. The mesher optimizations are sound but complex, introducing caching logic that needs validation in production.
  • Pay close attention to tidy3d/components/grid/mesher.py for the complex caching and vectorization logic, and monitor downstream effects of disabling cleanup in tidy3d/components/grid/corner_finder.py

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/grid/corner_finder.py 5/5 Added constants N_SHAPELY_QUAD_SEGS and SHAPELY_CLEANUP = False to disable polygon cleanup in corner detection, passing these to merging_geometries_on_plane
tidy3d/components/geometry/base.py 4/5 Added cleanup and quad_segs parameters throughout the geometry intersection methods, threading them through the entire hierarchy to control polygon cleanup behavior
tidy3d/components/geometry/primitives.py 5/5 Renamed constant _N_SHAPELY_QUAD_SEGS to _N_SHAPELY_QUAD_SEGS_VISUALIZATION, and updated circular shape intersection methods to use the quad_segs parameter
tidy3d/components/grid/mesher.py 4/5 Performance optimization: added cached min_steps tracking, replaced O(n*m) operations with O(n log m) binary search using bisect and np.searchsorted, vectorized containment checks

Sequence Diagram

sequenceDiagram
    participant CF as CornerFinderSpec
    participant MGP as merging_geometries_on_plane
    participant BX as Box.intersections_with
    participant Geom as Geometry.intersections_plane
    participant TP as to_polygon_list
    
    Note over CF: SHAPELY_CLEANUP equals False<br/>N_SHAPELY_QUAD_SEGS equals 8
    
    CF->>MGP: Call with cleanup False quad_segs 8
    MGP->>BX: Call intersections_with cleanup False quad_segs 8
    BX->>Geom: Call intersections_plane cleanup False quad_segs 8
    
    alt Circular shapes Sphere or Cylinder
        Geom->>Geom: Use quad_segs 8 for discretization
        Note over Geom: Low resolution for meshing<br/>versus 200 for visualization
    else Non-circular shapes
        Geom->>Geom: quad_segs parameter ignored
    end
    
    Geom-->>BX: Return list of Shapely objects
    BX-->>MGP: Return list of Shapely objects
    MGP->>TP: Call to_polygon_list with cleanup False
    
    alt cleanup True old behavior
        TP->>TP: Call cleanup_shapely_object<br/>Remove tiny features
    else cleanup False new behavior
        Note over TP: Skip cleanup step<br/>preserve all vertices
    end
    
    TP-->>MGP: Return list of Polygons
    MGP-->>CF: Return merged geometries
    
    CF->>CF: Call filter_collinear_vertices
    Note over CF: Identify corners based on<br/>angle_threshold parameter
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. tidy3d/components/geometry/base.py, line 2162-2172 (link)

    syntax: missing cleanup parameter documentation

  2. tidy3d/components/geometry/utils.py, line 96-113 (link)

    syntax: missing cleanup parameter in docstring

6 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Nov 24, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/components/geometry/base.py (81.8%): Missing lines 3196,3199
  • tidy3d/components/geometry/mesh.py (0.0%): Missing lines 644
  • tidy3d/components/geometry/polyslab.py (100%)
  • tidy3d/components/geometry/primitives.py (78.6%): Missing lines 488-489,510
  • tidy3d/components/geometry/utils.py (100%)
  • tidy3d/components/grid/corner_finder.py (100%)
  • tidy3d/components/grid/grid_spec.py (100%)
  • tidy3d/components/grid/mesher.py (96.8%): Missing lines 810

Summary

  • Total: 62 lines
  • Missing: 7 lines
  • Coverage: 88%

tidy3d/components/geometry/base.py

  3192             List of 2D shapes that intersect plane.
  3193             For more details refer to
  3194             `Shapely's Documentation <https://shapely.readthedocs.io/en/stable/project.html>`_.
  3195         """
! 3196         a = self.geometry_a.intersections_tilted_plane(
  3197             normal, origin, to_2D, cleanup=cleanup, quad_segs=quad_segs
  3198         )
! 3199         b = self.geometry_b.intersections_tilted_plane(
  3200             normal, origin, to_2D, cleanup=cleanup, quad_segs=quad_segs
  3201         )
  3202         geom_a = shapely.unary_union([Geometry.evaluate_inf_shape(g) for g in a])
  3203         geom_b = shapely.unary_union([Geometry.evaluate_inf_shape(g) for g in b])

tidy3d/components/geometry/mesh.py

  640                     "Unable to compute 'TriangleMesh.intersections_plane'. "
  641                     "Using bounding box instead."
  642                 )
  643             log.warning(f"Error encountered: {e}")
! 644             return self.bounding_box.intersections_plane(x=x, y=y, z=z, cleanup=cleanup)
  645 
  646     def inside(
  647         self, x: np.ndarray[float], y: np.ndarray[float], z: np.ndarray[float]
  648     ) -> np.ndarray[bool]:

tidy3d/components/geometry/primitives.py

  484             `Shapely's Documentation <https://shapely.readthedocs.io/en/stable/project.html>`_.
  485         """
  486         import trimesh
  487 
! 488         if quad_segs is None:
! 489             quad_segs = _N_SHAPELY_QUAD_SEGS_VISUALIZATION
  490 
  491         z0, (x0, y0) = self.pop_axis(self.center, self.axis)
  492         half_length = self.finite_length_axis / 2

  506             elif r_bot < 0 or np.isclose(r_bot, 0):
  507                 r_bot = 0
  508                 z_bot = z0 + self._radius_z(z0) / self._tanq
  509 
! 510         angles = np.linspace(0, 2 * np.pi, quad_segs * 4 + 1)
  511 
  512         if r_bot > 0:
  513             x_bot = x0 + r_bot * np.cos(angles)
  514             y_bot = y0 + r_bot * np.sin(angles)

tidy3d/components/grid/mesher.py

  806         ArrayFloat1D
  807             Boolean array where True means the position is contained in at least one box.
  808         """
  809         if not contained_2d:
! 810             return np.zeros(len(normal_positions), dtype=bool)
  811 
  812         # Stack all z-bounds: shape (num_boxes, 2)
  813         z_bounds = np.array([[box[0, 2], box[1, 2]] for box in contained_2d])

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

looks good! any unit tests that could be added?

@weiliangjin2021
Copy link
Collaborator Author

looks good! any unit tests that could be added?

Sure, will add more.

In the meantime, I just added another optional variable.

@dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute I'm not sure if this edge case matters to gap refinement or the convexity analysis for minimal feature size.

gap refinement doesn't capture gaps and strips thinner than this limit relative to the grid size https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/components/grid/grid_spec.py#L45

@weiliangjin2021
Copy link
Collaborator Author

@greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4108-speed-up-or-disable-polygon-cleanup-in-plane-intersection-in-corner-finder branch from 943b0ba to e3e70d9 Compare November 25, 2025 05:09
@weiliangjin2021
Copy link
Collaborator Author

@greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Nov 25, 2025
@daquinteroflex daquinteroflex removed this pull request from the merge queue due to a manual request Nov 25, 2025
@daquinteroflex daquinteroflex force-pushed the weiliang/FXC-4108-speed-up-or-disable-polygon-cleanup-in-plane-intersection-in-corner-finder branch from e3e70d9 to bca0a59 Compare November 25, 2025 21:15
@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4108-speed-up-or-disable-polygon-cleanup-in-plane-intersection-in-corner-finder branch from bca0a59 to 05ef72c Compare November 26, 2025 00:03
@weiliangjin2021 weiliangjin2021 added the rc3 3rd pre-release label Nov 26, 2025
@weiliangjin2021
Copy link
Collaborator Author

Sorry for pushing some many pieces in one PR, but it's hard to separate them as they will have conflict with each other. @momchil-flex could you review the changes in mesher.py?

After avoiding some computing repetition in #3019, one simulation still takes > 10min for meshing, as it contains too many geometries (it's a giant PCB). Now with this PR on top of the previous PR, it takes <3 min.

@weiliangjin2021
Copy link
Collaborator Author

@greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4108-speed-up-or-disable-polygon-cleanup-in-plane-intersection-in-corner-finder branch from 05ef72c to bf0e9a1 Compare November 26, 2025 04:41
Corner finder: reduce the number of vertices for smoothly curved cross section

Mesher and gap refinement: performance improvement in parse_structures
@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4108-speed-up-or-disable-polygon-cleanup-in-plane-intersection-in-corner-finder branch from bf0e9a1 to 0a1e13b Compare November 26, 2025 15:43
@weiliangjin2021 weiliangjin2021 removed the rc3 3rd pre-release label Nov 26, 2025
Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Looked at mesher only, but looks great!

@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Nov 27, 2025
Merged via the queue into develop with commit a385e55 Nov 27, 2025
33 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/FXC-4108-speed-up-or-disable-polygon-cleanup-in-plane-intersection-in-corner-finder branch November 27, 2025 22:41
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.

4 participants