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

dmarek-flex · 2025-10-31T02:12:01Z

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

Greptile Overview

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 in mode_spec.py to specify voltage patterns across conductors using coordinate points, structure names, or geometric regions (arrays)
Added terminal_specs field to MicrowaveModeSpec for mode selection and ordering
Implemented validation methods in ModePlaneAnalyzer: _convert_terminal_specifications_to_candidate_geometry(), _identify_conductor_voltage_sets(), and _validate_conductor_voltage_configurations()
Added early validation in ModeSolver._validate_mode_plane_analysis() to catch configuration errors before submission
Refactored Simulation._validate_microwave_mode_specs() to _validate_microwave_mode_plane_analysis() for consistency
Added _post_process_modes_with_terminal_specs() placeholder that raises SetupError for local mode solver (backend implementation pending)

Implementation notes:

The null check for structure name lookup is correctly implemented (lines 325-329 in mode_plane_analyzer.py)
Previous comments about assert statements and ValueError/SetupError mismatches were referring to outdated code - the current implementation uses proper error handling throughout
Tests comprehensively cover validation, error cases, and various terminal specification types
The terminal specification conversion logic correctly handles tuples (points), strings (structure names), and arrays (points/lines/polygons based on shape)

Confidence Score: 4/5

This PR is safe to merge with minimal risk - the implementation is well-tested and adds new functionality without breaking existing features
Score reflects solid implementation with comprehensive test coverage. The code properly handles error cases, has appropriate validation, and follows the codebase's error handling patterns. Previous review comments appear to be outdated and don't reflect the current state of the code.
No files require special attention - all implementations follow best practices and have good test coverage

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/components/microwave/mode_spec.py	4/5	Added `TerminalSpec` class and `terminal_specs` field to `MicrowaveModeSpec` for voltage-based mode selection. Clean implementation with good validation logic.
tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py	2/5	Added terminal specification conversion and validation methods. Contains assert statements (lines 319, 325) that should be proper error handling, and null check for structure name lookup is present (lines 325-329).
tidy3d/components/mode/mode_solver.py	4/5	Added `_validate_mode_plane_analysis()` and `_post_process_modes_with_terminal_specs()` methods for early validation. Also fixed return type annotation from list to tuple.

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()
    
    alt terminal_specs is not None
        ModeSolver->>ModePlaneAnalyzer: Create analyzer instance
        ModePlaneAnalyzer->>ModePlaneAnalyzer: conductor_bounding_boxes
        ModePlaneAnalyzer->>ModePlaneAnalyzer: conductor_shapes (cached)
        ModePlaneAnalyzer-->>ModeSolver: conductor info
        
        ModeSolver->>ModePlaneAnalyzer: _identify_conductor_voltage_sets()
        ModePlaneAnalyzer->>ModePlaneAnalyzer: _convert_terminal_specifications_to_candidate_geometry()
        ModePlaneAnalyzer->>TerminalSpec: Read plus/minus terminals
        TerminalSpec-->>ModePlaneAnalyzer: terminal identifiers
        
        alt terminal is tuple
            ModePlaneAnalyzer->>ModePlaneAnalyzer: Create Point
        else terminal is string
            ModePlaneAnalyzer->>ModePlaneAnalyzer: Find structure by name
            ModePlaneAnalyzer->>ModePlaneAnalyzer: Get intersections with geometry
        else terminal is array
            ModePlaneAnalyzer->>ModePlaneAnalyzer: Create Point/LineString/Polygon
        end
        
        ModePlaneAnalyzer->>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
        
        alt validation fails
            ModePlaneAnalyzer-->>ModeSolver: Raise SetupError
            ModeSolver-->>User: "TerminalSpec was not setup correctly"
        end
    end
    
    User->>ModeSolver: solve() / access .data
    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

greptile-apps

_{7 files reviewed, 8 comments}

_{Edit Code Review Agent Settings | Greptile}

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py

tests/test_components/test_microwave.py

tidy3d/components/microwave/mode_spec.py

github-actions · 2025-11-04T21:42:47Z

Diff Coverage

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

tidy3d/init.py (100%)
tidy3d/components/microwave/data/monitor_data.py (100%)
tidy3d/components/microwave/mode_spec.py (100%)
tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py (100%)
tidy3d/components/mode/mode_solver.py (100%)
tidy3d/components/simulation.py (100%)

Summary

Total: 168 lines
Missing: 0 lines
Coverage: 100%

dmarek-flex · 2025-11-11T03:24:11Z

another review @greptile

greptile-apps

_{15 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

weiliangjin2021

_get_isolated_conductors_as_shapely in ModePlaneAnalyzer can be slow as it seems to take all structures, and interior_disjoint_geometries defaults to False. Could you filter structures similar to

tidy3d/tidy3d/components/grid/grid_spec.py

Line 1456 in 4d5efb9

    
           structures_intersect = [s for s in structure_list if self.intersects(s.geometry)]

, and pass interior_disjoint_geometries variable that defaults toTrue`?

weiliangjin2021 · 2025-11-12T17:08:54Z

tidy3d/components/microwave/data/monitor_data.py

        "been used to set up the monitor or mode solver.",
    )

+    def _is_transmission_line_mode(self, mode_index: int) -> bool:


should we name it _is_quasi_tem_mode to be more specific?

Yes, and since I need to add this mode_type categorization anyways in the other PR. I will try to quickly generalize to determine TEM/quasiTEM,TE,TM.

weiliangjin2021 · 2025-11-12T17:11:35Z

tidy3d/components/microwave/data/monitor_data.py

    )

+    def _is_transmission_line_mode(self, mode_index: int) -> bool:
+        """Check if a mode qualifies as a quasi-TEM transmission line mode.


add here that it's checked at lowest frequency

weiliangjin2021 · 2025-11-12T17:46:34Z

tidy3d/components/microwave/mode_spec.py

+        """Validate conductor identification inputs."""
+
+        # If it's a string or tuple, pass through
+        if isinstance(val, (str, tuple)):


could you elaborate on why tuple is always valid?

i see, it's supposed to be a 2d point. that needs validation as well.

weiliangjin2021 · 2025-11-12T17:48:26Z

tidy3d/components/microwave/mode_spec.py

+        description="Optional tuple of terminal specifications for mode selection and ordering in "
+        "transmission line systems. Each 'TerminalSpec' defines the desired voltage pattern (which conductors "
+        "should be positive vs. negative) for a mode. When provided, the mode solver automatically reorders "
+        "computed modes to match the terminal specification order and applies phase corrections to ensure "


sometimes it's called phase alignment

weiliangjin2021 · 2025-11-12T19:21:42Z

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py

+                raise SetupError(
+                    f"No conductor found intersecting with the {terminal_type}_terminal. "
+                    "Please ensure that your terminal specification (coordinate, line, or polygon) "
+                    "intersects with at least one conductive structure in the mode plane. "


"intersects with exactly one conductive structure"

weiliangjin2021 · 2025-11-12T19:26:46Z

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py

+                plus_indices = [
+                    i for i, geom in enumerate(conductor_shapely) if geom.intersects(plus_terminal)
+                ]
+                validate_conductor_intersection(plus_indices, "plus")


what about multiple terminal_specs intersect the same conductor?

dbochkov-flexcompute

Looks very good to me, some minor comments

dbochkov-flexcompute · 2025-11-12T23:18:33Z

tidy3d/components/microwave/mode_spec.py

+        "of vertices defining a geometric region (point for N=1, line for N=2, polygon for N>2). "
+        "The coordinate or region should lie within the desired conductor in the mode plane.",
+    )
+


I think it is checked at some point, but would it make sense to add a cheap validator here that checks that when plus and minus terminals are converted to sets their intersection is empty? I think that might require to convert numpy array into list or tuples. Just to invalidate grossly incorrectly setups like TerminalSpec(plus_terminals=["ground"], minus_terminals=["ground"])

dbochkov-flexcompute · 2025-11-12T23:20:02Z

tidy3d/components/microwave/mode_spec.py

+    ... )
+    """
+
+    plus_terminals: tuple[ConductorIdentifierType, ...] = pd.Field(


Definitely optional, but I think plus and minus would also be acceptable here instead of plus_terminals and minus_terminals

dbochkov-flexcompute · 2025-11-12T23:28:46Z

tidy3d/components/microwave/mode_spec.py

        "ignored for the associated mode.",
    )

+    terminal_specs: Optional[tuple[TerminalSpec, ...]] = pd.Field(


is there any restrictions on the length of this tuple, like < num_modes? It would also be good to clarify in description whether each terminal_spec is for a separate mode

dmarek-flex · 2025-11-13T15:19:20Z

and pass interior_disjoint_geometries variable that defaults toTrue`?

From understanding, I need that False. I need the shapes of conductors that will ultimately be simulated, users might place dielectrics after conductors to mask parts of them. In other words, geometries can overlap with different properties in the plane.

weiliangjin2021 · 2025-11-13T15:55:52Z

and pass interior_disjoint_geometries variable that defaults toTrue`?

From understanding, I need that False. I need the shapes of conductors that will ultimately be simulated, users might place dielectrics after conductors to mask parts of them. In other words, geometries can overlap with different properties in the plane.

My understanding is that we'll follow standard conventions that metal structure always override dielectrics. For mask, users need to manually apply boolean operations to metal structures.

daquinteroflex · 2025-11-13T16:42:25Z

tidy3d/components/mode/mode_solver.py

+                mode_plane_analyzer = ModePlaneAnalyzer(
+                    center=plane.center,
+                    size=plane.size,
+                    field_data_colocated=colocate,
+                    structures=sim.volumetric_structures,
+                    grid=sim.grid,
+                    symmetry=sim.symmetry,
+                    sim_box=sim.simulation_geometry,
+                )
+                _ = mode_plane_analyzer.conductor_bounding_boxes


What benefit do we get by having ModePlaneAnalyzer being a class rather than a function? Just how you're envisioning this to grow

daquinteroflex · 2025-11-13T16:42:59Z

tidy3d/components/mode/mode_solver.py

+                    voltage_sets = mode_plane_analyzer._identify_conductor_voltage_sets(
+                        mode_spec.terminal_specs
+                    )
+                    mode_plane_analyzer._validate_conductor_voltage_configurations(voltage_sets)


The thing is that if it were a function it does not require schema changes really? And don't see the class itself bring much value?

daquinteroflex · 2025-11-13T16:45:24Z

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py

+# where the first set contains positive terminals and the second set contains negative terminals.
+VoltageSets = tuple[set[int], set[int]]
+# Conversion of user-supplied TerminalSpec into shapely geometries which are used to find intersecting conductors
+TerminalSpecShapes = tuple[list[Shapely], list[Shapely]]


So this is what you want to extend to more inputs ultimately?

daquinteroflex · 2025-11-13T16:46:24Z

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py


+# Type for holding sets of indices associated with conductors,
+# where the first set contains positive terminals and the second set contains negative terminals.
+VoltageSets = tuple[set[int], set[int]]


Do we feel this is going to be extensible fully?

daquinteroflex · 2025-11-13T16:48:13Z

tidy3d/components/microwave/path_integrals/mode_plane_analyzer.py

+    def mode_symmetry(self) -> tuple[Symmetry, Symmetry, Symmetry]:
+        """Mode symmetry considering simulation box and simulation symmetry."""
+        return self._get_mode_symmetry(self.sim_box, self.symmetry)
+
+    @cached_property
+    def mode_limits(self) -> Bound:
+        """Mode plane bounds restricted to final grid positions.
+
+        Mode profiles are calculated on a grid which is expanded from the monitor size
+        to the closest grid boundaries, taking into account symmetry conditions.
+        """
+        return self._get_mode_limits(self.grid, self.mode_symmetry)


So this type of code structure gives me an impression of trying to store a state but then we have the initialization latency ultimately, and it depends on the applications

dmarek-flex self-assigned this Oct 31, 2025

dmarek-flex added RF rc3 3rd pre-release labels Oct 31, 2025

dmarek-flex force-pushed the dmarek/FXC-3725-voltage-based-mode-selection branch 2 times, most recently from c6b771b to fac5487 Compare November 4, 2025 21:15

dmarek-flex marked this pull request as ready for review November 4, 2025 21:20

greptile-apps bot reviewed Nov 4, 2025

View reviewed changes

dmarek-flex force-pushed the dmarek/FXC-3725-voltage-based-mode-selection branch from fac5487 to 90a1ac0 Compare November 4, 2025 21:25

dmarek-flex requested a review from weiliangjin2021 as a code owner November 6, 2025 21:45

dmarek-flex force-pushed the dmarek/FXC-3725-voltage-based-mode-selection branch 3 times, most recently from 70ed291 to 0349e69 Compare November 10, 2025 21:37

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

a26413b

dmarek-flex force-pushed the dmarek/FXC-3725-voltage-based-mode-selection branch from 0349e69 to a26413b Compare November 11, 2025 03:16

dmarek-flex requested a review from dbochkov-flexcompute November 11, 2025 03:26

greptile-apps bot reviewed Nov 11, 2025

View reviewed changes

weiliangjin2021 reviewed Nov 12, 2025

View reviewed changes

dbochkov-flexcompute reviewed Nov 12, 2025

View reviewed changes

daquinteroflex reviewed Nov 13, 2025

View reviewed changes

dmarek-flex removed the rc3 3rd pre-release label Nov 13, 2025

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

dmarek-flex commented Oct 31, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Diff Coverage

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

Summary

Uh oh!

dmarek-flex commented Nov 11, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

weiliangjin2021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmarek-flex commented Nov 13, 2025

Uh oh!

weiliangjin2021 commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daquinteroflex Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

dmarek-flex commented Oct 31, 2025 •

edited by greptile-apps bot

Loading

github-actions bot commented Nov 4, 2025 •

edited

Loading

daquinteroflex Nov 13, 2025 •

edited

Loading