Add interpolation to EME #2973

caseyflex · 2025-11-06T21:22:52Z

Interpolation in EME can greatly speed up multi-frequency EME simulations.

Greptile Overview

Greptile Summary

This PR adds frequency interpolation support to EME simulations, enabling significant performance improvements for multi-frequency simulations by computing modes at a reduced set of frequencies and interpolating to obtain results at all requested frequencies.

Key Changes:

Replaced track_freq field with interp_spec field in EMEModeSpec, defaulting to Chebyshev sampling with 3 points
Added _monitor_mode_freqs() method to compute actual frequencies based on interpolation settings
Implemented interpolation logic in smatrix_in_basis() for mode overlaps when both modes have matching interp_spec
Changed sampling_points() comparison from > to >= to handle edge case where num_points equals frequency count
Added early return optimization in FreqDataset.interp_in_freq() when frequencies already match
Commented out validate_interp_num_points warnings in monitor and mode solver

Issues Found:

Docstring in EMEModeSpec.interp_spec incorrectly references removed sort_spec.track_freq field (should reference parent class's sort_spec if tracking is actually required)
Validation warnings commented out without explanation - unclear why this was done and whether it impacts user experience

Confidence Score: 3/5

This PR introduces a useful optimization feature but has documentation issues and unclear design decisions around validation that need attention before merging.
Score reflects: (1) a critical documentation bug where the docstring references a non-existent field that was removed in this same PR, which could confuse users; (2) commented-out code without explanation violates coding standards and makes intent unclear; (3) the core interpolation logic appears sound but the approach of accessing modes.monitor.freqs should be verified for all mode data types; (4) the feature itself is well-designed and fills a real performance need.
Pay close attention to tidy3d/components/eme/grid.py (incorrect docstring reference) and tidy3d/components/eme/data/sim_data.py (verify monitor.freqs access pattern)

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/components/mode_spec.py	4/5	Changed comparison from > to >= in sampling_points to return all freqs when num_points equals freq count
tidy3d/components/eme/grid.py	3/5	Replaced `track_freq` field with `interp_spec` field in EMEModeSpec, enabling frequency interpolation with default Chebyshev sampling
tidy3d/components/eme/data/sim_data.py	3/5	Added interpolation logic for mode overlaps when both modes have matching interp_spec, otherwise uses interpolated_copy

Sequence Diagram

sequenceDiagram
    participant User
    participant EMESimulation
    participant EMEModeSpec
    participant ModeInterpSpec
    participant ModeSolver
    participant EMESimulationData
    
    User->>EMESimulation: Create simulation with EMEModeSpec
    EMESimulation->>EMEModeSpec: Get interp_spec (default: Cheb 3 points)
    
    Note over EMESimulation,ModeInterpSpec: Frequency Sampling Phase
    EMESimulation->>EMEModeSpec: _monitor_mode_freqs()
    EMEModeSpec->>ModeInterpSpec: sampling_points(freqs)
    ModeInterpSpec-->>EMEModeSpec: Return reduced freq set
    EMEModeSpec-->>EMESimulation: Actual frequencies to compute
    
    Note over ModeSolver: Mode Computation Phase
    EMESimulation->>ModeSolver: Solve modes at sampled freqs
    ModeSolver-->>EMESimulation: Mode data at reduced freqs
    
    Note over EMESimulationData: S-Matrix Computation Phase
    User->>EMESimulationData: smatrix_in_basis(modes1, modes2)
    EMESimulationData->>EMESimulationData: Check interp_spec1 and interp_spec2
    
    alt Both have matching interp_spec
        EMESimulationData->>EMESimulationData: Compute overlaps at sampled freqs
        EMESimulationData->>EMESimulationData: _interp_dataarray_in_freq() on overlaps
        EMESimulationData-->>User: Interpolated S-matrix
    else Different or None interp_spec
        EMESimulationData->>EMESimulationData: Use interpolated_copy
        EMESimulationData->>EMESimulationData: Interpolate modes to all freqs first
        EMESimulationData->>EMESimulationData: Compute overlaps at all freqs
        EMESimulationData-->>User: Full frequency S-matrix
    end

Context used (3)

Rule from dashboard - Keep docstrings and comments synchronized with code changes to ensure they are always accurate and n... (source)
Rule from dashboard - Remove commented-out or obsolete code; rely on version control for history. (source)
Rule from dashboard - Add comments to explain the reasoning (why) behind complex, non-obvious, or workaround code decision... (source)

greptile-apps

_{9 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-12T23:35:49Z

tidy3d/components/eme/grid.py

+    interp_spec: Optional[ModeInterpSpec] = pd.Field(
+        ModeInterpSpec.cheb(num_points=3, reduce_data=True),
+        title="Mode frequency interpolation specification",
+        description="Specification for computing modes at a reduced set of frequencies and "
+        "interpolating to obtain results at all requested frequencies. This can significantly "
+        "reduce computational cost for broadband simulations where modes vary smoothly with "
+        "frequency. Requires frequency tracking to be enabled (``sort_spec.track_freq`` must "
+        "not be ``None``) to ensure consistent mode ordering across frequencies.",
    )


logic: The description states "Requires frequency tracking to be enabled (sort_spec.track_freq must not be None)", but EMEModeSpec no longer has a track_freq field (it was removed in this PR). The docstring should be updated to reflect the actual requirements. Consider checking the parent ModeSpec class's sort_spec field instead if tracking is needed.

Prompt To Fix With AI

This is a comment left during a code review. Path: tidy3d/components/eme/grid.py Line: 29:37 Comment: **logic:** The description states "Requires frequency tracking to be enabled (`sort_spec.track_freq` must not be `None`)", but `EMEModeSpec` no longer has a `track_freq` field (it was removed in this PR). The docstring should be updated to reflect the actual requirements. Consider checking the parent `ModeSpec` class's `sort_spec` field instead if tracking is needed. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2025-11-12T23:35:50Z

tidy3d/components/mode_spec.py

+        if self.num_points >= len(freqs):
            return freqs


style: changed from > to >= but now the validation warning is commented out in monitor.py:440 and mode_solver.py:518-519. Users won't be warned when num_points >= len(freqs), which defeats the purpose of this optimization. Either restore the warning or add a comment explaining why it's acceptable.

Prompt To Fix With AI

This is a comment left during a code review. Path: tidy3d/components/mode_spec.py Line: 436:437 Comment: **style:** changed from `>` to `>=` but now the validation warning is commented out in monitor.py:440 and mode_solver.py:518-519. Users won't be warned when `num_points >= len(freqs)`, which defeats the purpose of this optimization. Either restore the warning or add a comment explaining why it's acceptable. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2025-11-12T23:35:51Z

tidy3d/components/monitor.py

        return bytes_single

-    _warn_interp_num_points = validate_interp_num_points()
+    # _warn_interp_num_points = validate_interp_num_points()


style: validation commented out instead of removed. Add a comment explaining why (e.g., "Warning disabled because EME now defaults to interpolation" or similar context).

Prompt To Fix With AI

This is a comment left during a code review. Path: tidy3d/components/monitor.py Line: 440:440 Comment: **style:** validation commented out instead of removed. Add a comment explaining why (e.g., "Warning disabled because EME now defaults to interpolation" or similar context). How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2025-11-12T23:35:51Z

tidy3d/components/mode/mode_solver.py

+        # if self.mode_spec.interp_spec is not None:
+        #     _warn_interp_num_points(self.mode_spec.interp_spec, self.freqs)


style: validation commented out. Add a comment explaining why this warning was disabled.

Prompt To Fix With AI

This is a comment left during a code review. Path: tidy3d/components/mode/mode_solver.py Line: 518:519 Comment: **style:** validation commented out. Add a comment explaining why this warning was disabled. How can I resolve this? If you propose a fix, please make it concise.

caseyflex · 2025-11-12T23:34:46Z

tidy3d/components/eme/grid.py

-        "mode tracking based on the lowest, central, or highest frequency. "
-        "If ``None`` no mode tracking is performed, which is the default for best performance.",
+    interp_spec: Optional[ModeInterpSpec] = pd.Field(
+        ModeInterpSpec.cheb(num_points=3, reduce_data=True),


@momchil-flex We could change this to be GVD based (i.e. polynomial with three sampling points concentrated around the central frequency). But I'm not sure that's better in general. It can lower the accuracy towards the edge of the spectrum, and I haven't had a problem with mode tracking. And the user can customize this if needed

caseyflex · 2025-11-12T23:35:47Z

tidy3d/components/mode/mode_solver.py

-        if self.mode_spec.interp_spec is not None:
-            _warn_interp_num_points(self.mode_spec.interp_spec, self.freqs)
+        # if self.mode_spec.interp_spec is not None:
+        #     _warn_interp_num_points(self.mode_spec.interp_spec, self.freqs)


@dbochkov-flexcompute Do we need these warnings? They are annoying in an EME simulation where interpolation is the default, and falling back to the actual frequencies specified is even better (not worth warning about necessarily)

caseyflex · 2025-11-12T23:36:13Z

tidy3d/components/mode_spec.py

        >>> sampling_freqs = interp_spec.sampling_points(freqs)
        """
-        if self.num_points > len(freqs):
+        if self.num_points >= len(freqs):


@dbochkov-flexcompute not really needed for EME, but should we make this change?

caseyflex · 2025-11-12T23:46:13Z

tidy3d/components/monitor.py

        return val

+    @property
+    def _stored_freqs(self) -> list[float]:


@dbochkov-flexcompute this is to update _storage_size_solver for ModeSolverMonitor (although the full freqs will still be used for ModeMonitor)

ah, yeah, I updated ModeMonitor and ModeSolverMonitor directly but forgot about AbstractModeMonitor

caseyflex mentioned this pull request Nov 6, 2025

Add interpolation to EME #2936

Closed

caseyflex requested a review from dbochkov-flexcompute November 6, 2025 21:24

dbochkov-flexcompute force-pushed the daniil/FXC-3351-Downsample-frequencies-in-mode-monitors branch 2 times, most recently from bdd7b09 to aedc3a9 Compare November 11, 2025 02:19

caseyflex force-pushed the casey/emeinterp3 branch from ead0cff to 0b23b05 Compare November 11, 2025 21:04

dbochkov-flexcompute force-pushed the daniil/FXC-3351-Downsample-frequencies-in-mode-monitors branch 4 times, most recently from 3571d01 to 73e8ac0 Compare November 12, 2025 17:31

Base automatically changed from daniil/FXC-3351-Downsample-frequencies-in-mode-monitors to develop November 12, 2025 19:29

caseyflex force-pushed the casey/emeinterp3 branch 4 times, most recently from 0c72776 to 3676cf9 Compare November 12, 2025 23:26

caseyflex marked this pull request as ready for review November 12, 2025 23:32

caseyflex requested a review from momchil-flex November 12, 2025 23:32

greptile-apps bot reviewed Nov 12, 2025

View reviewed changes

caseyflex commented Nov 12, 2025

View reviewed changes

caseyflex force-pushed the casey/emeinterp3 branch from 3676cf9 to 455b032 Compare November 12, 2025 23:40

Add interpolation to EME

5a642c9

caseyflex force-pushed the casey/emeinterp3 branch from 455b032 to 5a642c9 Compare November 12, 2025 23:44

caseyflex commented Nov 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add interpolation to EME #2973

Add interpolation to EME #2973

Uh oh!

caseyflex commented Nov 6, 2025 •

edited by greptile-apps bot

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Nov 12, 2025

Uh oh!

greptile-apps bot Nov 12, 2025

Uh oh!

greptile-apps bot Nov 12, 2025

Uh oh!

greptile-apps bot Nov 12, 2025

Uh oh!

caseyflex Nov 12, 2025

Uh oh!

caseyflex Nov 12, 2025

Uh oh!

caseyflex Nov 12, 2025

Uh oh!

caseyflex Nov 12, 2025

Uh oh!

dbochkov-flexcompute Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# if self.mode_spec.interp_spec is not None:
		# _warn_interp_num_points(self.mode_spec.interp_spec, self.freqs)

Add interpolation to EME #2973

Are you sure you want to change the base?

Add interpolation to EME #2973

Uh oh!

Conversation

caseyflex commented Nov 6, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Context used (3)

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

caseyflex Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

caseyflex Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

caseyflex Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

caseyflex Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

dbochkov-flexcompute Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caseyflex commented Nov 6, 2025 •

edited by greptile-apps bot

Loading