Skip to content

Conversation

@dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Nov 24, 2025

Something that caused issues in notebook. The plan is to take another careful look at it between rc3 and official 2.10, but disable for now

Greptile Overview

Greptile Summary

Disabled default low frequency smoothing in TerminalComponentModeler by changing the low_freq_smoothing field default from ModelerLowFrequencySmoothingSpec() to None.

  • Removed the DEFAULT_LOW_FREQUENCY_SMOOTHING_SPEC constant
  • Changed default value of low_freq_smoothing field from an instance to None
  • Low frequency smoothing can still be explicitly enabled by passing a ModelerLowFrequencySmoothingSpec instance
  • The conditional check on line 474 ensures backward compatibility for users who explicitly set this parameter

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk as it's a simple default value change
  • The change is straightforward and well-contained: removing a default constant and changing a field default from an instance to None. The existing conditional check on line 474 ensures the code handles None gracefully. Score is 4 instead of 5 due to missing changelog entry for this user-facing behavior change.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/plugins/smatrix/component_modelers/terminal.py 4/5 Removed default value for low_freq_smoothing field, changing from ModelerLowFrequencySmoothingSpec() to None

Sequence Diagram

sequenceDiagram
    participant User
    participant TerminalComponentModeler
    participant Simulation
    participant ModeMonitor
    
    User->>TerminalComponentModeler: Create modeler (low_freq_smoothing=None by default)
    TerminalComponentModeler->>TerminalComponentModeler: Check low_freq_smoothing field
    
    alt low_freq_smoothing is None (NEW behavior)
        TerminalComponentModeler->>Simulation: Create simulation without low freq smoothing
        Note over TerminalComponentModeler,Simulation: No extrapolation applied
    else low_freq_smoothing is set
        TerminalComponentModeler->>ModeMonitor: Get mode monitors
        TerminalComponentModeler->>Simulation: Create LowFrequencySmoothingSpec
        TerminalComponentModeler->>Simulation: Apply smoothing to mode monitors
        Note over TerminalComponentModeler,Simulation: Extrapolation applied at low frequencies
    end
    
    Simulation->>User: Return configured simulation
Loading

Context used:

  • Rule from dashboard - Require a changelog entry for any PR that is not purely an internal refactor. (source)

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.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

Diff Coverage

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

No lines with coverage information in this diff.

@dbochkov-flexcompute dbochkov-flexcompute force-pushed the hotfix/disable-default-low-freq-smoothing branch from bd96f57 to 262c25f Compare November 24, 2025 22:56
@daquinteroflex daquinteroflex added this pull request to the merge queue Nov 25, 2025
@daquinteroflex daquinteroflex removed this pull request from the merge queue due to a manual request Nov 25, 2025
@daquinteroflex daquinteroflex force-pushed the hotfix/disable-default-low-freq-smoothing branch from 262c25f to 40a52a3 Compare November 25, 2025 19:17
@daquinteroflex daquinteroflex force-pushed the hotfix/disable-default-low-freq-smoothing branch from 40a52a3 to e0f062b Compare November 25, 2025 20:01
@daquinteroflex daquinteroflex merged commit 7b879bd into develop Nov 25, 2025
19 checks passed
@daquinteroflex daquinteroflex deleted the hotfix/disable-default-low-freq-smoothing branch November 25, 2025 21:14
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.

5 participants