Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Sep 25, 2025

Greptile Overview

Updated On: 2025-10-16 20:12:05 UTC

Greptile Summary

This PR implements multimodal waveport support in the S-matrix component modeler, enabling waveguide ports to handle multiple propagating modes simultaneously. The changes refactor the WavePort class to use MicrowaveModeSpec instead of manual impedance calculations, introduce a network indexing system that creates separate entries for each port-mode combination, and update the analysis pipeline to work with NetworkIndex identifiers rather than simple port references.

Key architectural changes include replacing the single mode_index field with a multimodal approach, switching from TerminalPortType to NetworkIndex for port identification, and implementing caching optimizations for voltage/current calculations to avoid redundant computations across modes of the same port. The refactoring leverages existing microwave components for transmission line calculations while maintaining backward compatibility with lumped ports.

Important Files Changed

Changed Files
Filename Score Overview
tidy3d/plugins/smatrix/ports/wave.py 4/5 Major refactoring to support multimodal operation using MicrowaveModeSpec and removing manual impedance calculations
tidy3d/plugins/smatrix/component_modelers/terminal.py 4/5 Core network indexing changes to create separate entries for each port-mode combination
tidy3d/plugins/smatrix/analysis/terminal.py 4/5 Added caching optimization for voltage/current calculations and mode_index parameter support
tidy3d/plugins/smatrix/data/terminal.py 4/5 Updated method signature from TerminalPortType to NetworkIndex for multimodal support
tidy3d/plugins/smatrix/component_modelers/base.py 5/5 Removed automatic mode_index assignment to require explicit specification
tidy3d/plugins/smatrix/analysis/antenna.py 4/5 Refactored to use NetworkIndex-based processing instead of port-based iteration
tests/test_plugins/smatrix/test_terminal_component_modeler.py 3/5 Updated test suite with incomplete validation migration and duplicate code blocks
tests/test_plugins/smatrix/terminal_component_modeler_def.py 4/5 Updated WavePort configuration to use MicrowaveModeSpec with CustomImpedanceSpec
tests/utils.py 4/5 Added data generator functions for microwave monitor data with transmission line support

Confidence score: 3/5

  • This PR requires careful review due to significant architectural changes affecting the S-matrix analysis pipeline
  • Score reflects the comprehensive nature of the refactoring with multiple interdependent changes, incomplete test validation migration, and potential for runtime issues if network indexing logic has edge cases
  • Pay close attention to test files with TODO comments and duplicate code blocks, and verify that all validation logic has been properly migrated to the new API

Sequence Diagram

sequenceDiagram
    participant User
    participant TerminalComponentModeler
    participant WavePort
    participant MicrowaveModeSpec
    participant Simulation
    participant TerminalComponentModelerData
    participant SMatrixCalculation

    User->>TerminalComponentModeler: "Create modeler with multimodal wave ports"
    TerminalComponentModeler->>WavePort: "Initialize with MicrowaveModeSpec"
    WavePort->>MicrowaveModeSpec: "Configure num_modes > 1"
    
    User->>TerminalComponentModeler: "Generate simulation dictionary"
    TerminalComponentModeler->>TerminalComponentModeler: "Build network_dict mapping"
    Note over TerminalComponentModeler: Maps each mode to unique NetworkIndex
    
    loop For each mode in each WavePort
        TerminalComponentModeler->>WavePort: "Create source for mode_index"
        WavePort->>Simulation: "Add ModeSource with specific mode"
        TerminalComponentModeler->>WavePort: "Create monitors for mode"
        WavePort->>Simulation: "Add MicrowaveModeMonitor"
    end
    
    User->>TerminalComponentModeler: "Run simulations"
    loop For each simulation task
        TerminalComponentModeler->>Simulation: "Execute FDTD simulation"
        Simulation->>TerminalComponentModelerData: "Return SimulationData"
    end
    
    User->>TerminalComponentModelerData: "Calculate S-matrix"
    TerminalComponentModelerData->>SMatrixCalculation: "Process multimodal data"
    
    loop For each port and mode combination
        SMatrixCalculation->>WavePort: "Compute voltage/current for mode"
        WavePort->>SMatrixCalculation: "Return modal V/I data"
        SMatrixCalculation->>SMatrixCalculation: "Convert to wave amplitudes"
    end
    
    SMatrixCalculation->>TerminalComponentModelerData: "Return complete S-matrix"
    TerminalComponentModelerData->>User: "Provide multimodal S-parameters"
Loading

@dmarek-flex dmarek-flex self-assigned this Sep 25, 2025
@dmarek-flex dmarek-flex added the RF label Sep 25, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/add_impedance_to_mode_data branch 3 times, most recently from 837a080 to c685b6b Compare October 2, 2025 17:28
@dmarek-flex dmarek-flex changed the title support multimodal waveports FXC-1575 support multimodal waveports Oct 7, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/add_impedance_to_mode_data branch 5 times, most recently from 40bcc17 to e9ca7aa Compare October 8, 2025 14:47
@dmarek-flex dmarek-flex force-pushed the dmarek/add_impedance_to_mode_data branch 4 times, most recently from 9aa1b9b to 7a5edfc Compare October 15, 2025 16:09
@dmarek-flex dmarek-flex force-pushed the dmarek/add_impedance_to_mode_data branch from 7a5edfc to c8f2694 Compare October 15, 2025 20:12
Base automatically changed from dmarek/add_impedance_to_mode_data to develop October 15, 2025 21:18
@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch 2 times, most recently from 3893644 to fa79e77 Compare October 16, 2025 20:11
@dmarek-flex dmarek-flex marked this pull request as ready for review October 16, 2025 20:11
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. tests/test_plugins/smatrix/test_terminal_component_modeler.py, line 799-817 (link)

    logic: This code block is duplicated from lines780-798. The duplicate should be removed.

9 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2025

Diff Coverage

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

  • tidy3d/components/microwave/mode_spec.py (100%)
  • tidy3d/components/microwave/path_integrals/integrals/current.py (100%)
  • tidy3d/components/microwave/path_integrals/specs/current.py (100%)
  • tidy3d/components/mode/mode_solver.py (100%)
  • tidy3d/components/simulation.py (100%)
  • tidy3d/plugins/smatrix/analysis/antenna.py (100%)
  • tidy3d/plugins/smatrix/analysis/terminal.py (100%)
  • tidy3d/plugins/smatrix/component_modelers/terminal.py (100%)
  • tidy3d/plugins/smatrix/data/terminal.py (100%)
  • tidy3d/plugins/smatrix/ports/wave.py (100%)

Summary

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

@dmarek-flex dmarek-flex changed the title FXC-1575 support multimodal waveports FXC-3724 support multimodal waveports Oct 17, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch 5 times, most recently from bb34bd1 to 25f3ea4 Compare October 20, 2025 19:40
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Looks quite solid. One question: consider that the transmission line supports 2 modes, and only mode_index=1 is involved in a modeling. Previously, I can setup a modeler by setting mode_index=1 in Waveport definition. Currently, with num_modes=2, simulations for both mode indices will be performed, unless one specify run_only parameters. Maybe it can be helpful to provide a convenient function to setup the previous behavior?

@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch 3 times, most recently from f0cf1fe to ecb4d0e Compare October 23, 2025 15:05
@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch 2 times, most recently from b7a312c to 65f5eae Compare October 23, 2025 19:18
Copy link
Collaborator

@daquinteroflex daquinteroflex left a comment

Choose a reason for hiding this comment

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

Looks good just a few comments

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Just one comment.

@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch 2 times, most recently from 0f024f8 to f44909b Compare October 28, 2025 18:01
Copy link
Contributor

@George-Guryev-flxcmp George-Guryev-flxcmp left a comment

Choose a reason for hiding this comment

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

Looks solid — great work! Just one note on de-embedding: in its current state, it doesn’t support multimode waveport de-embedding due to a dimensionality mismatch.

@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch from f44909b to 356fd49 Compare October 30, 2025 01:21
@dmarek-flex dmarek-flex force-pushed the dmarek/support_multimodal_waveports branch from 356fd49 to 6adb1d0 Compare October 30, 2025 13:33
@dmarek-flex dmarek-flex enabled auto-merge October 30, 2025 13:49
@dmarek-flex dmarek-flex added this pull request to the merge queue Oct 30, 2025
Merged via the queue into develop with commit 9138b28 Oct 30, 2025
26 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/support_multimodal_waveports branch October 30, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants