Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

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

Greptile Summary

This PR adds a new symmetric_pseudo option for s_param_def in TerminalComponentModeler, which provides an alternative S-parameter formulation equivalent to the "traveling wave" definition in scikit-rf. The implementation applies a different scaling factor (F = sqrt(1/(2*Z))) that ensures the resulting S-matrix is symmetric when port impedances differ.

Key changes:

  • Extended SParamDef type to include symmetric_pseudo as a valid literal value
  • Modified compute_F() to handle the new symmetric_pseudo scaling with proper mathematical documentation
  • Updated terminal_construct_smatrix() to route symmetric_pseudo requests to pseudo wave matrices (reusing existing infrastructure)
  • Added comprehensive error handling with consistent error messages across all modified functions
  • Extended s_to_z() conversion logic to properly handle symmetric_pseudo (treating it like pseudo for conjugate handling)
  • Added test coverage validating symmetric_pseudo against scikit-rf's "traveling" wave definition

Issues found:

  • Test file contains debugging print() statements that should be removed
  • Some test assertions are commented out and need to be either removed or uncommented and fixed

Confidence Score: 4/5

  • This PR is safe to merge after removing debugging code
  • The implementation is mathematically sound and follows established patterns in the codebase. Error handling is consistent and comprehensive. The only issues are temporary debugging artifacts (print statements and commented-out tests) that should be cleaned up before merge.
  • tests/test_plugins/smatrix/test_terminal_component_modeler.py needs cleanup of debugging code

Important Files Changed

Filename Overview
tidy3d/plugins/smatrix/utils.py implemented symmetric_pseudo scaling in compute_F() with proper documentation and error handling
tidy3d/plugins/smatrix/analysis/terminal.py added symmetric_pseudo case to terminal_construct_smatrix() with proper error handling
tests/test_plugins/smatrix/test_terminal_component_modeler.py added tests for symmetric_pseudo but contains debugging print statements and commented-out assertions that need cleanup

Sequence Diagram

sequenceDiagram
    participant User
    participant ModelerData as TerminalComponentModelerData
    participant Constructor as terminal_construct_smatrix
    participant Utils as utils.compute_F
    participant S2Z as utils.s_to_z

    User->>ModelerData: smatrix(s_param_def="symmetric_pseudo")
    ModelerData->>Constructor: terminal_construct_smatrix(s_param_def="symmetric_pseudo")
    
    alt symmetric_pseudo
        Constructor->>ModelerData: get port_pseudo_wave_matrices
        Note over Constructor: Reuses pseudo wave matrices<br/>with different scaling
    else power
        Constructor->>ModelerData: get port_power_wave_matrices
    else pseudo
        Constructor->>ModelerData: get port_pseudo_wave_matrices
    end
    
    ModelerData-->>Constructor: a_matrix, b_matrix
    Constructor->>Constructor: compute S = b @ inv(a)
    Constructor-->>ModelerData: S-matrix
    ModelerData-->>User: MicrowaveSMatrixData
    
    opt Convert to Z-matrix
        User->>ModelerData: s_to_z(s_param_def="symmetric_pseudo")
        ModelerData->>S2Z: s_to_z(S, reference, s_param_def)
        S2Z->>Utils: compute_F(Z, "symmetric_pseudo")
        Utils-->>S2Z: F = sqrt(1/(2*Z))
        Note over S2Z: Uses non-conjugate Zport<br/>(same as pseudo)
        S2Z->>S2Z: Z = solve(I - F^-1*S*F, ...)
        S2Z-->>ModelerData: Z-matrix
        ModelerData-->>User: Impedance matrix
    end
Loading

Context used:

  • Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)

@dmarek-flex dmarek-flex changed the title feat(rf): Added symmetric_pseudo option for s_param_def FXC-4570 feat(rf): Added symmetric_pseudo option for s_param_def Dec 19, 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.

5 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Dec 19, 2025

Diff Coverage

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

  • tidy3d/plugins/smatrix/analysis/terminal.py (100%)
  • tidy3d/plugins/smatrix/data/terminal.py (100%)
  • tidy3d/plugins/smatrix/types.py (100%)
  • tidy3d/plugins/smatrix/utils.py (100%)

Summary

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

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4570-add-symmetric-pseudo-sparam-def branch from e1cf778 to 034206b Compare December 19, 2025 18:33
@dmarek-flex dmarek-flex changed the title FXC-4570 feat(rf): Added symmetric_pseudo option for s_param_def FXC-4570 feat(rf): Added pseudo_symmetric option for s_param_def Dec 19, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4570-add-symmetric-pseudo-sparam-def branch 2 times, most recently from 79f7c77 to 8fb7784 Compare December 19, 2025 18:47
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4570-add-symmetric-pseudo-sparam-def branch from 8fb7784 to b86eaec Compare December 19, 2025 18:57
@dmarek-flex
Copy link
Contributor Author

dmarek-flex commented Dec 19, 2025

@weiliangjin2021 @yuanshen-flexcompute Still can be modified if you want but I enabled this symmetric version of pseudo waves and tried to put all the relevant info in one place. I have another issue to track the docs PR for summarizing the various S parameter amplitude definitions and their properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants