Skip to content

Conversation

@caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Nov 24, 2025

This PR:

  • Adds a new monitor and data classes to record cell interface s matrix data. This is normally used in the EME solver, but is sometimes needed for optimizing structures
  • Fixes the documentation a bit for frequency interpolation in EME
  • Changes the EME num_freqs validation to use sampling points, with a larger limit for the overall interpolated freqs
  • Changes default num_sweep from 1 to None for EMECoefficientMonitor

Note: the interface s matrices are not currently normalized. To normalize S21 for example, you need to multiply by sqrt(abs(real(flux2)/real(flux1))), where flux1 and flux2 are the flux in cell 1 and cell 2, respectively. In the future, we can add a flag normalize to this monitor and EMECoefficientMonitor, and maybe make it default to True.

Greptile Overview

Greptile Summary

This PR adds EMEInterfaceSMatrixMonitor to record cell-interface S-matrices in the EME solver, which is useful for structure optimization. The implementation follows established patterns in the codebase by creating the full class hierarchy (monitor, data array, dataset, and monitor data classes) and properly exporting them in the public API.

Key changes:

  • Added EMEInterfaceSMatrixMonitor with appropriate field overrides for interval_space, eme_cell_interval_space, and num_sweep
  • Created corresponding data classes: EMEInterfaceSMatrixDataArray (5D with dimensions for f, sweep_index, eme_cell_index, mode_index_out, mode_index_in), EMEInterfaceSMatrixDataset, and EMEInterfaceSMatrixData
  • Changed default num_sweep from 1 to None in EMECoefficientMonitor to record all sweep indices by default
  • Improved EME frequency validation to distinguish between sampling points (for mode solving) and total interpolated frequencies, raising the limit for the latter from 500 to 2000
  • Updated documentation to clarify frequency interpolation behavior in EME simulations
  • Added comprehensive tests for the new monitor and data classes

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation follows established patterns consistently, includes comprehensive tests, and all previous review comments have been addressed. The changes are well-structured with proper inheritance hierarchy, documentation, and API exports.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/eme/monitor.py 5/5 Added EMEInterfaceSMatrixMonitor class with fields and storage size calculation; moved num_sweep from base EMEMonitor to specific monitor classes
tidy3d/components/eme/data/monitor_data.py 5/5 Added EMEInterfaceSMatrixData class that combines monitor with dataset, following standard pattern
tidy3d/components/data/data_array.py 5/5 Added EMEInterfaceSMatrixDataArray class with 5-dimensional structure for interface S-matrix data
tidy3d/components/eme/data/dataset.py 5/5 Added EMEInterfaceSMatrixDataset with S11, S12, S21, S22 fields mirroring EMESMatrixDataset structure
tidy3d/components/eme/simulation.py 5/5 Updated frequency validation to use sampling points with higher limits for interpolated frequencies; improved documentation

Sequence Diagram

sequenceDiagram
    participant User
    participant EMESimulation
    participant EMEInterfaceSMatrixMonitor
    participant EMEInterfaceSMatrixData
    participant EMEInterfaceSMatrixDataset
    participant EMEInterfaceSMatrixDataArray

    User->>EMESimulation: Create simulation with EMEInterfaceSMatrixMonitor
    EMESimulation->>EMEInterfaceSMatrixMonitor: Validate monitor configuration
    EMEInterfaceSMatrixMonitor->>EMEInterfaceSMatrixMonitor: Check num_modes, freqs, num_sweep
    EMEInterfaceSMatrixMonitor->>EMESimulation: Calculate storage_size()
    
    User->>EMESimulation: Run simulation
    EMESimulation->>EMESimulation: Solve EME cells and compute interface S-matrices
    EMESimulation->>EMEInterfaceSMatrixDataArray: Create data arrays for S11, S12, S21, S22
    Note over EMEInterfaceSMatrixDataArray: Dimensions: (f, sweep_index, eme_cell_index, mode_index_out, mode_index_in)
    
    EMEInterfaceSMatrixDataArray->>EMEInterfaceSMatrixDataset: Populate S11, S12, S21, S22 fields
    EMEInterfaceSMatrixDataset->>EMEInterfaceSMatrixData: Combine with monitor reference
    EMEInterfaceSMatrixData->>User: Return simulation data
    
    User->>EMEInterfaceSMatrixData: Access interface S-matrices
    Note over User,EMEInterfaceSMatrixData: S-matrix at eme_cell_index=i<br/>is between cells i and i+1
Loading

@caseyflex caseyflex force-pushed the casey/emesmatrixmonitor branch from 63523e6 to 637e8d4 Compare November 24, 2025 19:45
@caseyflex caseyflex marked this pull request as ready for review November 24, 2025 19:46
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.

8 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the casey/emesmatrixmonitor branch from 637e8d4 to 2c77070 Compare November 24, 2025 19:53
@caseyflex
Copy link
Contributor Author

@greptile

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.

11 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the casey/emesmatrixmonitor branch 2 times, most recently from 457f0b8 to 2f737c7 Compare November 24, 2025 20:36
@caseyflex caseyflex force-pushed the casey/emesmatrixmonitor branch from 2f737c7 to b46cdad Compare November 24, 2025 20:40
@caseyflex
Copy link
Contributor Author

@greptile

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.

11 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/init.py (100%)
  • tidy3d/components/data/data_array.py (100%)
  • tidy3d/components/eme/data/dataset.py (100%)
  • tidy3d/components/eme/data/monitor_data.py (100%)
  • tidy3d/components/eme/monitor.py (100%)
  • tidy3d/components/eme/simulation.py (100%)

Summary

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

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Thanks @caseyflex just some minor comments!

Comment on lines +61 to +62
"""Dataset storing interface s-matrices.
The s-matrix at ``eme_cell_index=i`` is between cells ``i`` and ``i+1``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we should capitalize "S-matrix" here? I noticed the spelling is a bit inconsistent and I think capital S is most common.

Comment on lines +64 to +65
title="EME Interface S Matrix Monitor",
description="EME interface s matrix monitor associated with this data.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inconsistent spelling here too

@caseyflex
Copy link
Contributor Author

Thanks @caseyflex just some minor comments!

Thanks! Will incorporate these, but I’m going to change the api significantly after some discussions so will close this in favor of a new PR

@caseyflex
Copy link
Contributor Author

caseyflex commented Nov 25, 2025

Partially subsumed by #3032 (just the easy stuff)

The actual interface S matrices will come in a separate PR

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.

4 participants