Skip to content

Conversation

daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Aug 19, 2025

Greptile Summary

This PR implements immutable RF data structures for Tidy3D's microwave simulation capabilities. The changes consolidate RF data array creation logic into centralized utility functions and introduce specialized data array types for voltage, current, and impedance measurements across frequency, time, and modal domains.

The main architectural changes include:

  1. New RF Data Structures: Added comprehensive voltage, current, and impedance array classes (VoltageArray, CurrentArray, ImpedanceArray) with domain-specific subclasses (frequency, time, frequency-mode variants) in data_array.py

  2. Centralized Utility Functions: Moved data array creation logic from individual classes to centralized helper functions in smatrix/utils.py (_make_voltage_data_array, _make_current_data_array, _make_impedance_data_array)

  3. S-Parameter Definition Tracking: Added s_param_def field to MicrowaveSMatrixData to track whether scattering parameters use 'pseudo' or 'power' wave definitions

  4. Matrix Operations Refactoring: Moved matrix inversion from class methods to utility functions (port_array_inv) to support immutability patterns

  5. Type Safety Improvements: Updated return types throughout the RF modules to use specific type aliases (CurrentIntegralResultTypes, VoltageIntegralResultTypes, ImpedanceResultTypes) instead of generic types

These changes align with functional programming principles by moving operations from instance methods to pure utility functions, supporting the goal of making RF data structures immutable. The refactoring affects microwave path integrals, impedance calculations, and S-matrix computations across the RF simulation framework.

Important Files Changed

Click to expand file changes
Filename Score Overview
tidy3d/components/data/data_array.py 2/5 Added comprehensive RF data structures for voltage/current/impedance arrays with critical naming bugs in helper functions
tidy3d/plugins/smatrix/utils.py 3/5 Expanded with new RF data types and utility functions, includes duplicate imports and type annotation mismatches
tidy3d/plugins/microwave/custom_path_integrals.py 1/5 Refactored to use centralized utility functions but affected by critical bugs in the utility functions
tidy3d/plugins/microwave/path_integrals.py 4/5 Clean refactoring to use centralized utilities with improved type safety and code organization
tidy3d/plugins/microwave/impedance_calculator.py 4/5 Successfully consolidated impedance data array creation logic into centralized utility function
tidy3d/plugins/smatrix/data/terminal.py 2/5 Added S-parameter definition tracking but has duplicate SParamDef definition and incomplete implementation
tests/test_plugins/smatrix/test_terminal_component_modeler.py 4/5 Clean update to mock new utility function location instead of old class method

Confidence score: 1/5

  • This PR has critical implementation bugs that will cause runtime failures in RF simulation functionality
  • Score reflects serious issues including swapped return types in helper functions, duplicate definitions, and incomplete implementations
  • Pay close attention to tidy3d/components/data/data_array.py and tidy3d/plugins/smatrix/utils.py which contain the most critical bugs that need fixing before merge

Sequence Diagram

sequenceDiagram
    participant User
    participant TerminalComponentModeler
    participant SimulationRunner
    participant DataProcessor
    participant SMatrixCalculator

    User->>TerminalComponentModeler: "Create modeler with ports and simulation"
    TerminalComponentModeler->>TerminalComponentModeler: "Validate configuration"
    TerminalComponentModeler->>TerminalComponentModeler: "Generate simulation dictionary"
    
    User->>SimulationRunner: "Run component modeler simulations"
    loop For each port excitation
        SimulationRunner->>SimulationRunner: "Run FDTD simulation"
        SimulationRunner->>DataProcessor: "Extract field data"
        DataProcessor->>DataProcessor: "Compute voltage and current"
    end
    
    DataProcessor->>SMatrixCalculator: "Process all simulation results"
    SMatrixCalculator->>SMatrixCalculator: "Compute wave amplitudes"
    SMatrixCalculator->>SMatrixCalculator: "Calculate S-matrix"
    SMatrixCalculator->>SMatrixCalculator: "Validate impedance consistency"
    
    SMatrixCalculator-->>User: "Return S-matrix and impedance data"
Loading

@daquinteroflex daquinteroflex force-pushed the dario/rf_endpoints_update branch 6 times, most recently from d15e14a to 5681c5e Compare August 20, 2025 10:03
@daquinteroflex
Copy link
Collaborator Author

We can fix the missing issues on the other branch

@daquinteroflex daquinteroflex marked this pull request as ready for review August 27, 2025 10:25
@daquinteroflex daquinteroflex merged commit fead14d into dario/rf_endpoints_update Aug 27, 2025
@daquinteroflex daquinteroflex deleted the dario/rf_arrays branch August 27, 2025 10:25
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.

7 files reviewed, 4 comments

Edit Code Review Bot Settings | Greptile

s_param_def: SParamDef = pd.Field(
"pseudo",
title="Scattering Parameter Definition",
description="Whether to scattering parameters are defined using the 'pseudo' or 'power' wave definitions.",
Copy link

Choose a reason for hiding this comment

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

syntax: Grammar error: 'Whether to scattering parameters' should be 'Whether scattering parameters'

Suggested change
description="Whether to scattering parameters are defined using the 'pseudo' or 'power' wave definitions.",
description="Whether scattering parameters are defined using the 'pseudo' or 'power' wave definitions.",

@@ -16,6 +34,13 @@
from tidy3d.plugins.smatrix.network import SParamDef
from tidy3d.plugins.smatrix.ports.coaxial_lumped import CoaxialLumpedPort
from tidy3d.plugins.smatrix.ports.rectangular_lumped import LumpedPort
from tidy3d.plugins.smatrix.data.data_array import PortDataArray, TerminalPortDataArray
Copy link

Choose a reason for hiding this comment

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

syntax: Duplicate import - this line repeats the import from line 33

return cls.assign_data_attrs(cls(data=result.data, coords=result.coords))


def _make_voltage_data_array(result: DataArray) -> CurrentIntegralResultTypes:
Copy link

Choose a reason for hiding this comment

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

logic: Type annotation mismatch - function name suggests voltage but returns CurrentIntegralResultTypes

Suggested change
def _make_voltage_data_array(result: DataArray) -> CurrentIntegralResultTypes:
def _make_voltage_data_array(result: DataArray) -> VoltageIntegralResultTypes:

return cls.assign_data_attrs(cls(data=result.data, coords=result.coords))


def _make_current_data_array(result: DataArray) -> VoltageIntegralResultTypes:
Copy link

Choose a reason for hiding this comment

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

logic: Type annotation mismatch - function name suggests current but returns VoltageIntegralResultTypes

Suggested change
def _make_current_data_array(result: DataArray) -> VoltageIntegralResultTypes:
def _make_current_data_array(result: DataArray) -> CurrentIntegralResultTypes:

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