Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Dec 24, 2025

Adding more aggressive validation, so users do not get confusing errors later.

Greptile Summary

This PR enhances validation in AutoImpedanceSpec path generation by adding two critical checks that prevent confusing errors from occurring later in the simulation pipeline:

  1. Validation against ALL conductors: Changed the conductor intersection check from using filtered_conductor_shapely (which excludes conductors touching PEC boundaries) to isolated_conductor_shapely (all conductors). This prevents auto-generated path specifications from intersecting with ground planes or other conductors that were filtered out for being shorted to boundaries.

  2. Mode plane bounds validation: Added a new check to ensure bounding boxes don't extend outside the original mode plane bounds, which can happen with coarse grids causing snapped bounding boxes to expand beyond the intended region.

The changes include comprehensive test coverage with a parametrized test that validates both error scenarios using intentionally coarse grids.

Confidence Score: 4/5

  • This PR is safe to merge with one minor issue to address regarding error message formatting consistency
  • The logic changes are correct and well-tested. The fix properly addresses the issue by checking path intersections against all conductors (not just filtered ones) and validating bounds. Comprehensive tests cover both edge cases. However, there's one formatting inconsistency in error messages that should be corrected to follow project style guidelines
  • tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py needs a minor formatting fix in error messages

Important Files Changed

Filename Overview
tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py Added two critical validation checks: one to detect path intersections with ALL conductors (not just filtered ones), and another to ensure bounding boxes stay within mode plane bounds
tests/test_components/test_microwave.py Added comprehensive parametrized test covering both edge cases: ground plane intersection and mode plane boundary extension scenarios

Sequence Diagram

sequenceDiagram
    participant Client
    participant ModePlaneAnalyzer
    participant get_conductor_bounding_boxes
    participant _get_isolated_conductors_as_shapely
    participant _filter_conductors_touching_sim_bounds
    participant _check_box_intersects_with_conductors

    Client->>ModePlaneAnalyzer: get_conductor_bounding_boxes(structures, grid, symmetry, sim_box)
    ModePlaneAnalyzer->>get_conductor_bounding_boxes: Process conductors
    get_conductor_bounding_boxes->>_get_isolated_conductors_as_shapely: Extract all conductors from structures
    _get_isolated_conductors_as_shapely-->>get_conductor_bounding_boxes: isolated_conductor_shapely (all conductors)
    get_conductor_bounding_boxes->>_filter_conductors_touching_sim_bounds: Filter conductors touching boundaries
    _filter_conductors_touching_sim_bounds-->>get_conductor_bounding_boxes: filtered_conductor_shapely (subset)
    
    Note over get_conductor_bounding_boxes: Generate bounding boxes from<br/>filtered_conductor_shapely
    
    loop For each bounding_box
        get_conductor_bounding_boxes->>_check_box_intersects_with_conductors: Check against isolated_conductor_shapely (NEW: uses ALL conductors)
        _check_box_intersects_with_conductors-->>get_conductor_bounding_boxes: intersection_detected
        alt Intersection detected
            get_conductor_bounding_boxes->>Client: Raise SetupError (conductor intersection)
        end
    end
    
    loop For each bounding_box
        alt Box extends outside mode plane bounds (NEW validation)
            get_conductor_bounding_boxes->>Client: Raise SetupError (extends outside bounds)
        end
    end
    
    get_conductor_bounding_boxes-->>Client: Return (bounding_boxes, filtered_conductor_shapely)
Loading

@dmarek-flex dmarek-flex self-assigned this Dec 24, 2025
@dmarek-flex dmarek-flex added the RF label Dec 24, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4661-Improper-auto-path-integral-validation branch from 8087c22 to 284f98f Compare December 24, 2025 18:43
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 (1)

  1. tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py, line 222-224 (link)

    style: Error message contains 'Structure' with markdown-style single quotes. According to the style guide, error messages should not use markdown formatting, and code identifiers like class names should be wrapped with single quotes that render as plain text (not RST/markdown).

    This appears to be pre-existing, but since you're modifying nearby error messages, consider fixing for consistency:

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

    Context Used: Rule from dashboard - Do not use markdown formatting in exception or warning messages; use single quotes to highlight vari... (source)

3 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py (100%)

Summary

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

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4661-Improper-auto-path-integral-validation branch from 284f98f to 777c499 Compare December 24, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants