Skip to content

Conversation

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Nov 21, 2025

Greptile Overview

Greptile Summary

This PR optimizes meshing performance in gap refinement by implementing strategic caching of expensive geometry operations. The key improvements are:

  • Extracted and cached parse_structures results: The _parse_structures method is now called once per axis and its results (interval_coords and max_dl_list) are reused across all gap meshing iterations, eliminating redundant structure parsing
  • Cached merged geometries: Added _internal_layerrefinement_merged_geos cached property in Simulation to compute merged PEC geometries once and reuse them throughout gap meshing iterations
  • Cached corner detection: Pre-computes corners and convexity data via _internal_layerfinement_corners_and_convexity_2d cached property
  • Refactored geometry methods: Changed _is_inplane_bounded from a @cached_property to a regular method that accepts a Box parameter for more flexible geometry checking; added new _layer_box and _merged_geos helper methods
  • Code organization: Moved increment_float to a dedicated float_utils.py module and added slightly_enlarged_copy method to Box class

The optimization maintains correctness by passing cached data through the call chain rather than recomputing it. Tests have been updated to match the new signatures.

Confidence Score: 4/5

  • safe to merge with minor documentation improvements recommended
  • the performance optimizations are well-structured and the caching strategy is sound; tests have been updated appropriately; the main concern is a typo in docstring and potential clarity improvements in parameter documentation
  • tidy3d/components/grid/grid_spec.py:2963 - typo in docstring 'snappiung' should be 'snapping'; tidy3d/components/grid/grid_spec.py:1466 - typo 'vaccum' should be 'vacuum'

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/geometry/float_utils.py 5/5 new utility module extracting increment_float function from utils_2d.py for better code organization and reusability
tidy3d/components/grid/corner_finder.py 4/5 refactored corner finding methods to accept pre-computed merged_geos and added keep_metal_only parameter for performance optimization; updated docstrings from List/Tuple to list/tuple
tidy3d/components/grid/grid_spec.py 4/5 major performance optimization: extracted _parse_structures to cache interval coordinates/max_dl_list across gap meshing iterations; added caching of merged geometries; changed _is_inplane_bounded from cached_property to method; added _layer_box and _merged_geos methods; updated gap meshing to use cached data
tidy3d/components/simulation.py 5/5 added cached properties _internal_layerrefinement_boundary_types and _internal_layerrefinement_merged_geos to pre-compute and cache expensive geometry operations; updated method calls to pass cached data

Sequence Diagram

sequenceDiagram
    participant Sim as Simulation
    participant GS as GridSpec
    participant LRS as LayerRefinementSpec
    participant CF as CornerFinderSpec
    
    Note over Sim: Grid Generation with Gap Meshing
    
    Sim->>Sim: _internal_layerrefinement_boundary_types<br/>(cached_property)
    Sim->>Sim: _internal_layerrefinement_merged_geos<br/>(cached_property)
    loop for each layer
        Sim->>LRS: _merged_geos()
        LRS->>CF: _merged_pec_on_plane()
        CF-->>LRS: merged geometries
        LRS-->>Sim: cache merged_geos
    end
    
    Sim->>Sim: _internal_layerfinement_corners_and_convexity_2d<br/>(cached_property)
    loop for each layer
        Sim->>LRS: _corners_and_convexity_2d(merged_geos)
        LRS->>CF: _corners_and_convexity(merged_geos)
        CF-->>LRS: corners & convexity
        LRS-->>Sim: cache corners data
    end
    
    Sim->>GS: make_grid()
    GS->>GS: _make_grid_and_snapping_lines()
    
    Note over GS: Pre-compute parse_structures once
    loop for each axis
        GS->>GS: _parse_structures()
        Note right of GS: Cache interval_coords<br/>and max_dl_list
    end
    
    GS->>GS: _make_grid_one_iteration(cached_data)
    Note right of GS: Initial grid with<br/>cached parse results
    
    Note over GS: Gap Meshing Iterations
    loop gap_meshing_iters
        loop for each layer
            GS->>LRS: _resolve_gaps(grid, cached_merged_geos)
            Note right of LRS: Use cached merged_geos<br/>instead of recomputing
            LRS-->>GS: snapping_lines
        end
        
        alt new snapping lines found
            GS->>GS: _make_grid_one_iteration(cached_data)
            Note right of GS: Reuse cached parse_structures<br/>interval_coords & max_dl_list
            
            alt grid unchanged
                Note over GS: Early exit
            end
        else no new snapping lines
            Note over GS: Early exit
        end
    end
    
    GS-->>Sim: final grid
Loading

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4106-cached_parse_structures branch from 122fc45 to 48a77e8 Compare November 21, 2025 22:44
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, 2 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Nov 21, 2025

Diff Coverage

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

  • tidy3d/components/geometry/base.py (100%)
  • tidy3d/components/geometry/float_utils.py (100%)
  • tidy3d/components/geometry/utils_2d.py (100%)
  • tidy3d/components/grid/corner_finder.py (100%)
  • tidy3d/components/grid/grid_spec.py (92.4%): Missing lines 1727,1736,1781-1782,1784,2593,3072
  • tidy3d/components/simulation.py (100%)

Summary

  • Total: 133 lines
  • Missing: 7 lines
  • Coverage: 94%

tidy3d/components/grid/grid_spec.py

  1723         if cached_corners_and_convexity is None:
  1724             if cached_merged_geos is None:
  1725                 merged_geos = self._merged_geos(structure_list, sim_bounds, boundary_type)
  1726             else:
! 1727                 merged_geos = cached_merged_geos
  1728             inplane_points, convexity = self._corners_and_convexity_2d(
  1729                 merged_geos=merged_geos,
  1730                 structure_list=structure_list,
  1731                 ravel=False,

  1732                 sim_bounds=sim_bounds,
  1733                 boundary_type=boundary_type,
  1734             )
  1735         else:
! 1736             inplane_points, convexity = cached_corners_and_convexity
  1737 
  1738         dl_min = inf
  1739 
  1740         if self.corner_finder is None or self.corner_finder._no_min_dl_override:

  1777         """Inplane corners in 3D coordinate."""
  1778         if self.corner_finder is None:
  1779             return []
  1780         if cached_corners_and_convexity is None:
! 1781             if cached_merged_geos is None:
! 1782                 merged_geos = self._merged_geos(structure_list, sim_bounds, boundary_type)
  1783             else:
! 1784                 merged_geos = cached_merged_geos
  1785             inplane_points, _ = self._corners_and_convexity_2d(
  1786                 merged_geos=merged_geos,
  1787                 structure_list=structure_list,
  1788                 ravel=True,

  2589                     cached_data = None
  2590                 if cached_merged_geos is not None:
  2591                     cached_merged = cached_merged_geos[ind]
  2592                 else:
! 2593                     cached_merged = None
  2594                 snapping_points += layer_spec.generate_snapping_points(
  2595                     list(structures),
  2596                     sim_bounds,
  2597                     boundary_types,

  3068                         # use cached merged geometries if available, otherwise compute
  3069                         if cached_merged_geos is not None:
  3070                             merged_geos = cached_merged_geos[ind_layer]
  3071                         else:
! 3072                             merged_geos = layer_spec._merged_geos(
  3073                                 structures,
  3074                                 sim_bounds,
  3075                                 boundary_types,
  3076                             )

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4106-cached_parse_structures branch from 8add44f to c170e05 Compare November 25, 2025 16:54
@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-4106-cached_parse_structures branch from c170e05 to 020a499 Compare November 25, 2025 19:21
feat(meshing): gap refinement only insert snapping points
@daquinteroflex daquinteroflex force-pushed the weiliang/FXC-4106-cached_parse_structures branch from 020a499 to 164b9e6 Compare November 25, 2025 21:15
@weiliangjin2021 weiliangjin2021 added the rc3 3rd pre-release label Nov 26, 2025
@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Nov 26, 2025
Merged via the queue into develop with commit 2b6d21b Nov 26, 2025
19 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/FXC-4106-cached_parse_structures branch November 26, 2025 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rc3 3rd pre-release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants