feat(rf): FXC-3725 add voltage-based selection of modes to the ModeSolver

[dmarek-flex]

backend: https://github.com/flexcompute/compute/pull/2764

Greptile Overview

Updated On: 2025-11-04 21:24:30 UTC

Greptile Summary

This PR adds voltage-based mode selection capability to the ModeSolver through a new TerminalSpec class that allows users to specify which conductors should be at positive or negative voltage for transmission line modes.

Key changes:

• Added TerminalSpec class to specify voltage patterns across conductors using coordinate points, structure names, or geometric regions
• Added terminal_specs field to MicrowaveModeSpec for mode selection and ordering
• Implemented validation methods in ModePlaneAnalyzer to convert terminal specifications to conductor indices and validate configurations
• Added early validation in ModeSolver._validate_mode_plane_analysis() to catch configuration errors before submission
• Refactored Simulation._validate_microwave_mode_specs() to reuse validation logic

Critical issues found:

• Logic error in convert_terminal() (line 273-276): incorrect conditions for creating Point/LineString/Polygon geometries will cause runtime errors
• Missing null check when looking up structures by name (line 270-272): will raise AttributeError if structure not found
• Assert statements used instead of proper error handling (lines 319, 325): violates production code standards
• Test/code mismatch: tests expect ValueError but code raises SetupError

Confidence Score: 1/5

• This PR has critical logic errors that will cause runtime failures
• Multiple critical bugs in core logic: (1) geometry creation logic has incorrect conditionals that will create wrong shapely objects or fail, (2) missing null check will cause AttributeError when structure names don't match, (3) assert statements violate production code standards, (4) test expectations don't match actual exception types raised. These issues will definitely cause runtime errors.
• Primary focus on tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py (lines 266-276, 319, 325) and tests/test_components/test_microwave.py (lines 1820, 1829)

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py	1/5	Added terminal specification conversion methods with critical logic errors: missing null check for structure names, incorrect geometry creation logic, and assert statements instead of proper error handling
tests/test_components/test_microwave.py	1/5	Tests expect ValueError but code raises SetupError, causing test failures. Also modified existing test parameters without clear justification
tidy3d/components/microwave/mode_spec.py	3/5	Added TerminalSpec class with validation logic and new terminal_specs field. Minor formatting issues in description strings

Sequence Diagram

sequenceDiagram
    participant User
    participant ModeSolver
    participant MicrowaveModeSpec
    participant ModePlaneAnalyzer
    participant TerminalSpec
    
    User->>ModeSolver: Create with terminal_specs
    ModeSolver->>ModeSolver: _post_init_validators()
    ModeSolver->>ModeSolver: _validate_mode_plane_analysis()
    ModeSolver->>ModePlaneAnalyzer: get_conductor_bounding_boxes()
    ModePlaneAnalyzer-->>ModeSolver: conductor_shapes
    
    alt terminal_specs is not None
        ModeSolver->>ModePlaneAnalyzer: _convert_terminal_specifications_to_candidate_geometry()
        ModePlaneAnalyzer->>TerminalSpec: Read plus/minus terminals
        TerminalSpec-->>ModePlaneAnalyzer: terminal identifiers
        ModePlaneAnalyzer->>ModePlaneAnalyzer: convert to Points/Lines/Polygons
        ModePlaneAnalyzer-->>ModeSolver: terminal_spec_shapes
        
        ModeSolver->>ModePlaneAnalyzer: _find_conductor_terminals()
        ModePlaneAnalyzer->>ModePlaneAnalyzer: Match terminals to conductors
        ModePlaneAnalyzer-->>ModeSolver: voltage_sets
        
        ModeSolver->>ModePlaneAnalyzer: _validate_conductor_voltage_configurations()
        ModePlaneAnalyzer->>ModePlaneAnalyzer: Check for conflicts/duplicates
    end
    
    User->>ModeSolver: solve()
    ModeSolver->>ModeSolver: _add_microwave_data()
    
    alt terminal_specs is not None
        ModeSolver->>ModeSolver: _post_process_modes_with_terminal_specs()
        Note over ModeSolver: Raises SetupError<br/>(not available locally)
    end
    
    ModeSolver->>ModeSolver: _make_path_integrals()
    ModeSolver-->>User: MicrowaveModeSolverData

Loading

[greptile-apps]

_{7 files reviewed, 8 comments}

_{Edit Code Review Agent Settings | Greptile}

[github-actions]

Diff Coverage

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

• tidy3d/init.py (100%)
• tidy3d/components/microwave/mode_spec.py (100%)
• tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py (29.3%): Missing lines 259,261,266-272,276-281,283,285-287,290,293-294,319,321-322,328-329,336-341,344-347,350-353
• tidy3d/components/mode/mode_solver.py (61.9%): Missing lines 381-382,387,390-392,1461,1470
• tidy3d/components/simulation.py (100%)

Summary

• Total: 112 lines
• Missing: 49 lines
• Coverage: 56%

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py

Lines 255-298

  255 
  256     def _convert_terminal_specifications_to_candidate_geometry(
  257         self, structures: list[Structure], terminal_specs: list[TerminalSpec]
  258     ) -> list[TerminalSpecShapes]:
! 259         def find_structure_by_name(target_name):
  260             """Find first element where name matches target_name."""
! 261             return next(
  262                 (item for item in structures if item.name is not None and item.name == target_name),
  263                 None,
  264             )
  265 
! 266         def convert_terminal(terminal_specifier) -> Shapely:
! 267             if isinstance(terminal_specifier, tuple):
! 268                 return Point(*terminal_specifier)
! 269             elif isinstance(terminal_specifier, str):
! 270                 structure = find_structure_by_name(terminal_specifier)
! 271                 if structure is None:
! 272                     raise SetupError(
  273                         f"No structure found with name '{terminal_specifier}'. "
  274                         "Please ensure that a `Structure` with the same name has been added to the simulation."
  275                     )
! 276                 shapes_plane = self.intersections_with(structure.geometry)
! 277                 return shapes_plane
! 278             elif terminal_specifier.shape[0] == 1:
! 279                 return Point(*terminal_specifier)
! 280             elif terminal_specifier.shape[0] == 2:
! 281                 return LineString(terminal_specifier)
  282             else:
! 283                 return Polygon(terminal_specifier)
  284 
! 285         terminal_spec_shapes = []
! 286         for terminal_spec in terminal_specs:
! 287             plus_spec_shapes = [
  288                 convert_terminal(plus_terminal) for plus_terminal in terminal_spec.plus_terminals
  289             ]
! 290             minus_spec_shapes = [
  291                 convert_terminal(minus_terminal) for minus_terminal in terminal_spec.minus_terminals
  292             ]
! 293             terminal_spec_shapes.append((plus_spec_shapes, minus_spec_shapes))
! 294         return terminal_spec_shapes
  295 
  296     def _find_conductor_terminals(
  297         self,
  298         conductor_shapely: list[Shapely],

---

Lines 315-326

  315         list[VoltageSets]
  316             List of (positive_conductor_indices, negative_conductor_indices) for each terminal spec.
  317         """
  318 
! 319         def validate_conductor_intersection(indices: list[int], terminal_type: str) -> None:
  320             """Validate that exactly one conductor intersects with the terminal."""
! 321             if len(indices) == 0:
! 322                 raise SetupError(
  323                     f"No conductor found intersecting with the {terminal_type}_terminal. "
  324                     "Please ensure that your terminal specification (coordinate, line, or polygon) "
  325                     "intersects with at least one conductive structure in the mode plane. "
  326                     "Check that the terminal coordinates are within the bounds of a conductor."

---

Lines 324-333

  324                     "Please ensure that your terminal specification (coordinate, line, or polygon) "
  325                     "intersects with at least one conductive structure in the mode plane. "
  326                     "Check that the terminal coordinates are within the bounds of a conductor."
  327                 )
! 328             elif len(indices) > 1:
! 329                 raise SetupError(
  330                     f"Multiple conductors ({len(indices)}) found intersecting with the {terminal_type}_terminal. "
  331                     "Please ensure that your terminal specification intersects with exactly one conductor. "
  332                     "Consider making your terminal specification more precise (e.g., using a smaller region or point) "
  333                     "to uniquely identify a single conductor."

---

Lines 332-357

  332                     "Consider making your terminal specification more precise (e.g., using a smaller region or point) "
  333                     "to uniquely identify a single conductor."
  334                 )
  335 
! 336         terminals = []
! 337         for term_spec in terminal_specs:
! 338             all_plus_indices = set()
! 339             all_minus_indices = set()
! 340             for plus_terminal in term_spec[0]:
! 341                 plus_indices = [
  342                     i for i, geom in enumerate(conductor_shapely) if geom.intersects(plus_terminal)
  343                 ]
! 344                 validate_conductor_intersection(plus_indices, "plus")
! 345                 all_plus_indices.update(plus_indices)
! 346             for minus_terminal in term_spec[1]:
! 347                 minus_indices = [
  348                     i for i, geom in enumerate(conductor_shapely) if geom.intersects(minus_terminal)
  349                 ]
! 350                 validate_conductor_intersection(minus_indices, "minus")
! 351                 all_minus_indices.update(minus_indices)
! 352             terminals.append((all_plus_indices, all_minus_indices))
! 353         return terminals
  354 
  355     def _validate_conductor_voltage_configurations(
  356         self, conductor_voltage_sets: list[VoltageSets]
  357     ) -> None:

---

tidy3d/components/mode/mode_solver.py

Lines 377-396

  377                     f"Failed to automatically place paths around conductors in the mode plane. {e!s}"
  378                 ) from e
  379 
  380             if mode_spec.terminal_specs is not None:
! 381                 try:
! 382                     terminal_spec_shapes = (
  383                         mode_plane_analyzer._convert_terminal_specifications_to_candidate_geometry(
  384                             sim.structures, mode_spec.terminal_specs
  385                         )
  386                     )
! 387                     voltage_sets = mode_plane_analyzer._find_conductor_terminals(
  388                         conductor_shapes, terminal_spec_shapes
  389                     )
! 390                     mode_plane_analyzer._validate_conductor_voltage_configurations(voltage_sets)
! 391                 except SetupError as e:
! 392                     raise SetupError(f"'TerminalSpec' was not setup correctly. {e!s}") from e
  393 
  394     @cached_property
  395     def normal_axis(self) -> Axis:
  396         """Axis normal to the mode plane."""

---

Lines 1457-1465

  1457         self,
  1458         mode_solver_data: MicrowaveModeSolverData,
  1459     ) -> MicrowaveModeSolverData:
  1460         """Select, sort and post process modes to match terminal specifications."""
! 1461         raise SetupError("Terminal-based mode setup is not available for the local mode solver.")
  1462 
  1463     def _add_microwave_data(
  1464         self, mode_solver_data: MicrowaveModeSolverData
  1465     ) -> MicrowaveModeSolverData:

---

Lines 1466-1474

  1466         """Calculate and add microwave data to ``mode_solver_data`` which uses the path specifications."""
  1467 
  1468         # Check if terminal specifications are present and will be used to drive the mode ordering and selection
  1469         if self.mode_spec.terminal_specs is not None:
! 1470             mode_solver_data = self._post_process_modes_with_terminal_specs(mode_solver_data)
  1471 
  1472         voltage_integrals, current_integrals = self._make_path_integrals()
  1473         # Need to operate on the full symmetry expanded fields
  1474         mode_solver_data_expanded = mode_solver_data.symmetry_expanded_copy

---