Skip to content

Conversation

@caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Nov 25, 2025

This PR:

  • 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

Subsumes the easy part of this PR:
#3024

Greptile Overview

Greptile Summary

This PR improves EME frequency validation and documentation by distinguishing between frequency sampling points (where modes are computed) and total interpolated frequencies. The validation now allows up to 2000 total frequencies when interpolation is enabled, but limits sampling points to 500, making the limits more practical for users who rely on mode interpolation.

Key changes:

  • Changed default num_sweep in EMECoefficientMonitor from 1 to None to record all sweep indices by default
  • Separated frequency validation into sampling points (MAX_NUM_SAMPLING_POINTS=500) vs total frequencies (MAX_NUM_FREQS=2000)
  • Added _num_sampling_points property to calculate actual mode solve points based on interpolation settings
  • Updated docstrings to clarify that modes are interpolated rather than recomputed at each frequency
  • Fixed minor typo: "eme cells" → "EME cells" in monitor docstrings
  • Added comprehensive tests for the new validation logic

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-structured, thoroughly tested, and address a legitimate user need. The validation logic properly distinguishes between sampling points and total frequencies, the default value change for num_sweep is backward-compatible (None is more permissive than 1), and comprehensive tests verify both interpolation-enabled and disabled scenarios.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/eme/simulation.py 5/5 Improved frequency validation logic to distinguish between sampling points and total frequencies, updated docstrings
tidy3d/components/eme/monitor.py 5/5 Changed default num_sweep from 1 to None in EMECoefficientMonitor, fixed typo in docstrings
tests/test_components/test_eme.py 5/5 Added comprehensive tests for new frequency validation logic with interpolation enabled/disabled

Sequence Diagram

sequenceDiagram
    participant User
    participant EMESimulation
    participant Validation
    participant EMEGrid
    participant ModeSpec
    
    User->>EMESimulation: Create simulation with freqs=[f1, f2, ..., fN]
    User->>EMESimulation: validate_pre_upload()
    EMESimulation->>Validation: _validate_size()
    Validation->>Validation: Check len(freqs) <= MAX_NUM_FREQS (2000)
    Validation->>EMESimulation: Get _num_sampling_points
    EMESimulation->>EMEGrid: Loop through mode_specs
    loop For each mode_spec
        EMEGrid->>ModeSpec: Get interp_spec
        alt interp_spec is None
            ModeSpec-->>EMESimulation: Add all freqs to set
        else interp_spec exists
            ModeSpec-->>EMESimulation: Add sampling_points(freqs) to set
        end
    end
    EMESimulation-->>Validation: Return len(unique sampling points)
    Validation->>Validation: Check sampling_points <= MAX_NUM_SAMPLING_POINTS (500)
    alt sampling_points > 500
        Validation-->>User: Raise SetupError
    else sampling_points > 20
        Validation-->>User: Log warning
    else
        Validation-->>User: Validation passes
    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.

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

7 files reviewed, 2 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.

7 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/components/eme/monitor.py (100%)
  • tidy3d/components/eme/simulation.py (100%)

Summary

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

@momchil-flex momchil-flex added this pull request to the merge queue Nov 26, 2025
Merged via the queue into develop with commit ec400b8 Nov 26, 2025
97 of 103 checks passed
@momchil-flex momchil-flex deleted the chore/casey/emefreq branch November 26, 2025 10:08
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