Skip to content

Conversation

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Nov 20, 2025

Greptile Overview

Greptile Summary

Fixed handling of polygons with holes (interiors) in the subdivide() function. When merged geometry results in a polygon with holes, the function now properly converts it into a PolySlab with subtraction operations using GeometryGroup. The fix also includes a refactoring of evaluate_inf_shape() to extract coordinate processing into a helper function for better code organization.

  • Fixed shapely_to_polyslab() helper function in utils_2d.py:125 to handle polygons with interior holes by creating a ClipOperation with difference operation
  • Refactored evaluate_inf_shape() in base.py:724 to use _processed_coords() helper for cleaner coordinate processing
  • Added comprehensive test case test_subdivide_geometry_group_with_polygon_holes() that verifies the fix by checking for ClipOperation with difference operation in finalized structures
  • Updated CHANGELOG.md with user-facing description of the fix

Confidence Score: 4/5

  • This PR is safe to merge with minor type annotation issue
  • The core logic fix is sound and properly handles polygon holes via subtraction operations. Comprehensive test coverage validates the fix. However, the return type annotation of shapely_to_polyslab() is incorrect - it declares -> PolySlab but can return ClipOperation when holes exist. This won't cause runtime issues since the outer function expects Geometry, but it creates type checking inconsistency.
  • Pay close attention to tidy3d/components/geometry/utils_2d.py due to type annotation mismatch

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/geometry/base.py 5/5 Refactored evaluate_inf_shape() to extract coordinate processing into helper function for better code organization
tidy3d/components/geometry/utils_2d.py 4/5 Fixed handling of polygons with holes by converting them to subtraction operations; has incorrect return type annotation
tests/test_components/test_geometry.py 5/5 Added comprehensive test for subdivide with geometry groups containing polygon holes

Sequence Diagram

sequenceDiagram
    participant Sim as Simulation
    participant Sub as subdivide()
    participant STP as shapely_to_polyslab()
    participant Geo as Geometry
    participant CO as ClipOperation
    
    Sim->>Sub: subdivide(geom, structures)
    Note over Sub: Process geometry with<br/>neighboring structures
    Sub->>Sub: Convert shapely polygons<br/>to multipolygon
    
    loop For each polygon
        Sub->>STP: shapely_to_polyslab(polygon, axis, center)
        
        alt Polygon has no holes
            STP->>Geo: Create PolySlab with exterior vertices
            Geo-->>STP: Return PolySlab
        else Polygon has holes (interiors)
            STP->>Geo: Create PolySlab with exterior vertices
            STP->>Geo: Create PolySlab for each interior
            STP->>Geo: Create GeometryGroup from interiors
            STP->>CO: polyslab - GeometryGroup
            Note over CO: Creates ClipOperation<br/>with operation="difference"
            CO-->>STP: Return ClipOperation
        end
        
        STP-->>Sub: Return Geometry (PolySlab or ClipOperation)
    end
    
    Sub-->>Sim: Return list of (Geometry, Structure, Structure)
    Note over Sim: Finalized structures contain<br/>ClipOperation with difference operation
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.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-4272-getting-different-results-with-and-without-geometry-group-for-pec branch from d31a23c to a546dea Compare November 21, 2025 00:01
@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/components/geometry/base.py (100%)
  • tidy3d/components/geometry/utils_2d.py (100%)

Summary

  • Total: 17 lines
  • Missing: 0 lines
  • Coverage: 100%

Copy link
Contributor

@dmarek-flex dmarek-flex left a comment

Choose a reason for hiding this comment

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

LGTM! 🖖

@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Nov 21, 2025
Merged via the queue into develop with commit 3037d0f Nov 21, 2025
45 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/FXC-4272-getting-different-results-with-and-without-geometry-group-for-pec branch November 21, 2025 17:45
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