Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Oct 1, 2025

Greptile Overview

Updated On: 2025-10-01 20:35:07 UTC

Summary

This PR fixes a critical bug in the `WavePort` class that occurred when users requested more than one mode in the `ModeSpec` during S-parameter calculations. The issue was in the mode selection logic within the wave port implementation.

The root cause was that mode selection was happening too early in the calculation pipeline - specifically in the _mode_voltage_coefficients and _mode_current_coefficients methods. This premature filtering prevented other parts of the code from accessing the full mode data needed for proper multi-mode calculations.

The fix relocates the mode selection from the coefficient calculation methods to the final voltage/current computation methods (compute_voltage and compute_current). Additionally, explicit mode_index selection is added when extracting forward and backward amplitudes. This architectural change ensures that:

  1. Coefficient calculations receive complete mode data, enabling proper multi-mode operation
  2. Final port-specific calculations still use only the requested mode
  3. Amplitude extraction explicitly selects the correct mode rather than relying on implicit selection

The changes integrate seamlessly with the existing S-matrix calculation infrastructure in the microwave plugin, maintaining backward compatibility while fixing the multi-mode scenario. A comprehensive test case has been added to prevent regression, and the fix is properly documented in the changelog.

Changed Files
Filename Score Overview
tidy3d/plugins/smatrix/ports/wave.py 4/5 Fixes mode selection bug by moving mode filtering from coefficient methods to final computation methods
tests/test_plugins/smatrix/test_terminal_component_modeler.py 5/5 Adds regression test to validate S-matrix shape with multiple modes requested
CHANGELOG.md 5/5 Documents the bug fix in the unreleased section following proper changelog conventions

Confidence score: 4/5

  • This PR addresses a specific bug with a targeted fix that maintains existing functionality while resolving the multi-mode issue
  • Score reflects the architectural nature of moving mode selection logic, which requires careful validation but appears well-implemented
  • Pay close attention to tidy3d/plugins/smatrix/ports/wave.py to ensure the mode selection timing doesn't introduce edge cases

Sequence Diagram

sequenceDiagram
    participant User
    participant TerminalComponentModeler as TCM
    participant WavePort
    participant ModeMonitor
    participant ModeSource
    participant Simulation
    participant ModeData
    participant ImpedanceCalculator as ImpCalc

    User->>TCM: "Create modeler with WavePort(mode_spec=ModeSpec(num_modes>1))"
    TCM->>WavePort: "Initialize with multi-mode ModeSpec"
    WavePort->>WavePort: "Store mode_spec and mode_index"
    
    User->>TCM: "Generate simulation dictionary"
    TCM->>WavePort: "to_source(source_time)"
    WavePort->>ModeSource: "Create source with mode_spec and mode_index"
    ModeSource-->>WavePort: "Return configured source"
    WavePort-->>TCM: "Return source"
    
    TCM->>WavePort: "to_monitors(freqs)"
    WavePort->>ModeMonitor: "Create monitor with full mode_spec"
    Note over ModeMonitor: BUG: Monitor requests all modes<br/>but only mode_index is used later
    ModeMonitor-->>WavePort: "Return monitor"
    WavePort-->>TCM: "Return [monitor]"
    
    TCM->>Simulation: "Create simulation with sources and monitors"
    Simulation-->>TCM: "Return configured simulation"
    
    User->>TCM: "Run simulation"
    TCM->>Simulation: "Execute FDTD simulation"
    Simulation->>ModeData: "Compute mode amplitudes for all modes"
    ModeData-->>Simulation: "Return multi-mode data"
    Simulation-->>TCM: "Return SimulationData"
    
    TCM->>WavePort: "compute_voltage(sim_data)"
    WavePort->>ModeData: "_isel(mode_index=[self.mode_index])"
    Note over ModeData: Filter to specific mode only
    ModeData-->>WavePort: "Return single mode data"
    WavePort->>WavePort: "_mode_voltage_coefficients(mode_data)"
    WavePort->>WavePort: "Calculate voltage from amplitudes"
    WavePort-->>TCM: "Return voltage array"
    
    TCM->>WavePort: "compute_current(sim_data)"
    WavePort->>ModeData: "_isel(mode_index=[self.mode_index])"
    ModeData-->>WavePort: "Return single mode data"
    WavePort->>WavePort: "_mode_current_coefficients(mode_data)"
    WavePort->>WavePort: "Calculate current from amplitudes"
    WavePort-->>TCM: "Return current array"
    
    TCM->>WavePort: "compute_port_impedance(sim_data)"
    WavePort->>ModeData: "_isel(mode_index=[self.mode_index])"
    ModeData-->>WavePort: "Return single mode data"
    WavePort->>ImpCalc: "compute_impedance(mode_data)"
    ImpCalc-->>WavePort: "Return impedance array"
    WavePort-->>TCM: "Return port impedance"
    
    TCM->>TCM: "Construct S-parameter matrix"
    TCM-->>User: "Return TerminalComponentModelerData"
Loading

@dmarek-flex dmarek-flex self-assigned this Oct 1, 2025
@dmarek-flex dmarek-flex added the RF label Oct 1, 2025
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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

Diff Coverage

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

  • tidy3d/plugins/smatrix/ports/wave.py (100%)

Summary

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

@dmarek-flex dmarek-flex added this pull request to the merge queue Oct 4, 2025
Merged via the queue into develop with commit 75fd628 Oct 4, 2025
34 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/fix-waveport-sparam-calc branch October 4, 2025 09:50
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