Skip to content

Conversation

dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Sep 25, 2025

Restarted this PR due to a substantial refactor. Comments from #2818 are addressed here as well

Greptile Overview

Updated On: 2025-09-25 17:37:45 UTC

Summary

This PR introduces low frequency extrapolation functionality for terminal component simulations. The implementation adds a new LowFrequencySmoothingSpec class that allows users to configure polynomial fitting parameters for extrapolating simulation results into lower frequency ranges.

Key changes:

  • New LowFrequencySmoothingSpec class with configurable parameters (sampling times, polynomial order, max deviation)
  • ModelerLowFrequencySmoothingSpec variant for terminal component modelers with automatic monitor detection
  • Integration with Simulation and TerminalComponentModeler classes
  • Comprehensive test coverage including edge cases and validation
  • Updated JSON schemas for proper API documentation
  • Clean refactoring of related utility code

Issues found:

  • Debug print statement left in simulation validator (critical - must be removed)
  • Incorrect exception type expected in one test (test expects ValueError but should expect SetupError)
  • Minor style improvements needed for empty collection checks

Confidence Score: 3/5

  • This PR is mostly safe to merge but requires fixing the debug print statement before deployment
  • Score reflects solid implementation with comprehensive tests, but critical issue with debug print statement prevents higher confidence
  • tidy3d/components/simulation.py requires immediate attention to remove debug print statement

Important Files Changed

File Analysis

Filename        Score        Overview
tidy3d/components/frequency_extrapolation.py 4/5 New module implementing LowFrequencySmoothingSpec with proper validation - minor style improvement needed
tidy3d/plugins/smatrix/component_modelers/terminal.py 4/5 Added ModelerLowFrequencySmoothingSpec and integration with TerminalComponentModeler - minor style improvement needed
tidy3d/components/simulation.py 3/5 Added low_freq_smoothing field and validation - debug print statement must be removed
tests/test_components/test_low_freq_smoothing.py 3/5 New test file with basic validation tests - incorrect exception type expected in one test

Sequence Diagram

sequenceDiagram
    participant User
    participant TerminalComponentModeler as TCM
    participant ModelerLowFrequencySmoothingSpec as MLFSS
    participant Simulation as Sim
    participant LowFrequencySmoothingSpec as LFSS
    participant ModeMonitor as MM

    User->>+TCM: Create with low_freq_smoothing parameter
    TCM->>+MLFSS: Initialize with sampling times, order, deviation
    MLFSS->>MLFSS: Validate min_sampling_time < max_sampling_time
    MLFSS-->>-TCM: Return validated spec
    
    User->>+TCM: Request sim_dict
    TCM->>TCM: Generate base simulation with monitors
    TCM->>+MM: Find mode monitors in simulation
    MM-->>-TCM: Return mode monitor names
    
    alt Mode monitors exist and low_freq_smoothing enabled
        TCM->>+LFSS: Create with mode monitor names and parameters
        LFSS->>LFSS: Validate monitors exist and are mode monitors
        LFSS-->>-TCM: Return simulation-specific spec
        TCM->>+Sim: Update simulation with low_freq_smoothing
        Sim->>Sim: Validate all monitors exist as ModeMonitors
        Sim-->>-TCM: Return updated simulation
    else No mode monitors or disabled
        TCM->>+Sim: Keep simulation without low_freq_smoothing
        Sim-->>-TCM: Return simulation
    end
    
    TCM-->>-User: Return sim_dict with configured simulations
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.

11 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

github-actions bot commented Sep 25, 2025

Diff Coverage

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

  • tidy3d/init.py (100%)
  • tidy3d/components/frequency_extrapolation.py (100%)
  • tidy3d/components/simulation.py (100%)
  • tidy3d/plugins/autograd/invdes/parametrizations.py (100%)
  • tidy3d/plugins/smatrix/init.py (100%)
  • tidy3d/plugins/smatrix/component_modelers/terminal.py (100%)

Summary

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

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Looks good overall!

@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/low-freq-extrapolation branch from 8e8b50e to 3257a0b Compare September 29, 2025 17:28
Copy link
Contributor

@George-Guryev-flxcmp George-Guryev-flxcmp left a comment

Choose a reason for hiding this comment

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

Looks good overall!

@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/low-freq-extrapolation branch 3 times, most recently from 471729b to e579b17 Compare October 6, 2025 21:54
@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/low-freq-extrapolation branch from e579b17 to 33d09e9 Compare October 9, 2025 05:26
@dbochkov-flexcompute dbochkov-flexcompute changed the title Low freq extrapolation Low freq extrapolation (FXC-2558) Oct 9, 2025
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.

3 participants