Skip to content

Conversation

@caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Dec 2, 2025

Replaces #3024.

This PR extends EMECoefficientDataset to store additional data:

  • cell interface S matrices
  • n_complex and flux of EME modes

Furthermore, instead of using a monitor, this data is always stored in EMESimulationData.coeffs (it is always computed and is not very large) as long as EMESimulation.store_coeffs is True (the default). So EMECoefficientMonitor is deprecated.

Greptile Overview

Greptile Summary

This PR extends the EME (Eigenmode Expansion) simulation capabilities by storing additional coefficient data directly in EMESimulationData.coeffs rather than requiring a separate monitor. The implementation adds new data array types (EMEInterfaceSMatrixDataArray, EMEFluxDataArray) and extends EMECoefficientDataset to store interface S-matrices, mode propagation indices, and flux values. The PR also deprecates EMECoefficientMonitor since the data is now always available when EMESimulation.store_coeffs=True.

Key changes include:

  • New EMEInterfaceSMatrixDataArray and EMEFluxDataArray classes with proper dimension definitions
  • Extended EMECoefficientDataset with optional fields for n_complex, flux, and interface_smatrices
  • Added normalized_copy property for flux-normalized coefficients
  • Added store_coeffs flag to EMESimulation (defaults to True)
  • Comprehensive test coverage for new functionality
  • Proper deprecation warning for EMECoefficientMonitor

Issues found:

  • Potential dimension mismatch in the normalization logic where eme_port_index dimension from coefficient arrays may not align properly with flux arrays during normalization

Confidence Score: 4/5

  • This PR is generally safe to merge with one potential dimension mismatch issue that should be verified
  • Score reflects solid implementation with comprehensive tests, proper deprecation handling, and good API design. However, there's a potential logical issue with dimension alignment in the normalization method that could cause runtime errors or incorrect results when normalizing coefficients across different ports. The rest of the implementation is clean and well-tested.
  • Pay close attention to tidy3d/components/eme/data/dataset.py - verify the normalization logic handles the eme_port_index dimension correctly

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/data/data_array.py 5/5 Added two new data array classes (EMEInterfaceSMatrixDataArray and EMEFluxDataArray) with proper dimensions and examples
tidy3d/components/eme/data/dataset.py 4/5 Extended EMECoefficientDataset with new fields and normalization logic - one potential dimension mismatch issue in normalization
tidy3d/components/eme/simulation.py 5/5 Added store_coeffs flag and deprecation warning for EMECoefficientMonitor - clean implementation
tests/test_components/test_eme.py 5/5 Added comprehensive tests for new data arrays and normalization logic - good test coverage

Sequence Diagram

sequenceDiagram
    participant User
    participant EMESimulation
    participant Solver as EME Solver
    participant EMESimulationData
    participant EMECoefficientDataset
    
    User->>EMESimulation: Create simulation with store_coeffs=True
    User->>Solver: Run EME simulation
    
    Note over Solver: Compute EME modes
    Solver->>Solver: Calculate A & B coefficients
    Solver->>Solver: Calculate mode n_complex
    Solver->>Solver: Calculate mode flux
    Solver->>Solver: Calculate interface S-matrices
    
    alt store_coeffs=True
        Solver->>EMECoefficientDataset: Create dataset with all coefficients
        EMECoefficientDataset->>EMECoefficientDataset: Store A, B, n_complex, flux, interface_smatrices
        Solver->>EMESimulationData: Attach coeffs dataset
    else store_coeffs=False
        Solver->>EMESimulationData: coeffs=None
    end
    
    EMESimulationData->>User: Return simulation results
    
    opt User requests normalized coefficients
        User->>EMECoefficientDataset: Access normalized_copy
        EMECoefficientDataset->>EMECoefficientDataset: Check flux is not None
        EMECoefficientDataset->>EMECoefficientDataset: Normalize A, B by sqrt(flux_out)
        EMECoefficientDataset->>EMECoefficientDataset: Normalize S12, S21 by sqrt(flux ratios)
        EMECoefficientDataset->>EMECoefficientDataset: Set flux=None to prevent double normalization
        EMECoefficientDataset->>User: Return normalized dataset
    end
Loading

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, 3 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Diff Coverage

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

  • tidy3d/components/data/data_array.py (100%)
  • tidy3d/components/eme/data/dataset.py (100%)
  • tidy3d/components/eme/data/sim_data.py (100%)
  • tidy3d/components/eme/simulation.py (100%)

Summary

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

@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.

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the casey/emelocaldata branch 3 times, most recently from d4e40a8 to 54f45d3 Compare December 2, 2025 23:15
@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.

9 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@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.

9 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@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.

9 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Nice!

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 looks good! Just some questions on compatibility.

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