Skip to content

Conversation

@daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Dec 5, 2025

Greptile Overview

Greptile Summary

This PR consolidates and improves RF license warnings across the codebase. The changes remove redundant per-class warning validators in favor of centralized warnings in Simulation._validate_rf_component_warnings(), update warning messages to be clearer and remove emojis (following style guidelines), and optimize the log_once mechanism to check for duplicates before message composition.

Key changes:

  • Removed _warn_rf_license root validator from MicrowaveBaseModel, TerminalComponentModeler, and TerminalComponentModelerData
  • Moved refactor warning from TerminalComponentModeler to base class AbstractComponentModeler to reduce duplication
  • Updated all RF license warning messages to standardized text without emojis
  • Optimized Logger._log() to check log_once cache before composing messages
  • Updated documentation to improve cross-references and correct autosummary paths

Potential issues:

  • Test test_RF_license_suppression() may fail since it expects warnings when instantiating MicrowaveModeSpec() directly, but the validator that emitted these warnings was removed
  • The log_once optimization changes behavior: duplicate messages are no longer captured in the message stack, which could affect tests that check captured warnings

Confidence Score: 3/5

  • This PR is mostly safe but requires test verification before merging
  • The refactoring is well-intentioned and consolidates warnings properly, but removing the _warn_rf_license validator from MicrowaveBaseModel will likely break test_RF_license_suppression() which expects warnings when instantiating RF classes directly. The log_once optimization also changes message capture behavior. These issues need verification before merge.
  • Pay close attention to tidy3d/components/microwave/base.py and tidy3d/log.py - test suite must be run to verify no regressions

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/microwave/base.py 3/5 Removed _warn_rf_license root validator entirely; _default_without_license_warning method remains but warning mechanism removed from base class
tidy3d/log.py 5/5 Moved log_once check before message composition to prevent duplicate message processing - good optimization
tidy3d/plugins/smatrix/component_modelers/base.py 5/5 Added refactor warning to base class; improved message with migration link and proper formatting

Sequence Diagram

sequenceDiagram
    participant User
    participant MicrowaveClass
    participant MicrowaveBaseModel
    participant Logger
    participant Simulation
    
    Note over User,Simulation: Before PR (removed validators)
    User->>MicrowaveClass: Instantiate (e.g., MicrowaveModeSpec())
    MicrowaveClass->>MicrowaveBaseModel: __init__()
    MicrowaveBaseModel->>MicrowaveBaseModel: _warn_rf_license() [REMOVED]
    MicrowaveBaseModel-->>Logger: log.warning() [NO LONGER CALLED]
    
    Note over User,Simulation: After PR (centralized warnings)
    User->>Simulation: Create Simulation with RF components
    Simulation->>Simulation: _validate_rf_component_warnings()
    Simulation->>Logger: log.warning() with new message
    
    Note over User,Simulation: Factory method (unchanged)
    User->>MicrowaveClass: _default_without_license_warning()
    MicrowaveClass->>MicrowaveBaseModel: Temporarily set suppress flag
    MicrowaveBaseModel->>MicrowaveClass: __init__() (no warning)
    MicrowaveBaseModel->>MicrowaveBaseModel: Restore suppress flag
Loading

Context used (3)

  • Rule from dashboard - Do not use markdown formatting in exception or warning messages; use single quotes to highlight vari... (source)
  • Rule from dashboard - Make log messages and warnings informative by including relevant context and enclosing variable name... (source)
  • Rule from dashboard - Keep docstrings and comments synchronized with code changes to ensure they are always accurate and n... (source)

@daquinteroflex daquinteroflex changed the title fix: rf license warnings and docs fix: rf license warnings and docs (FXC-4427) Dec 5, 2025
@daquinteroflex daquinteroflex force-pushed the dario/fix_rf_license_warning branch from b6a0d3a to 49b316f Compare December 5, 2025 16:50
@daquinteroflex daquinteroflex marked this pull request as ready for review December 10, 2025 10:18
@daquinteroflex daquinteroflex force-pushed the dario/fix_rf_license_warning branch from 49b316f to f1b6312 Compare December 10, 2025 10:18
@daquinteroflex
Copy link
Collaborator Author

So once the docs PR is merged from the merge queue will rebase this and maybe @dmarek-flex you can help review?

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.

16 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@daquinteroflex daquinteroflex force-pushed the dario/fix_rf_license_warning branch from 8c1d59f to 8e6f3bb Compare December 10, 2025 11:05
@daquinteroflex
Copy link
Collaborator Author

@greptile review again

@daquinteroflex daquinteroflex force-pushed the dario/fix_rf_license_warning branch from 8e6f3bb to a31e79f Compare December 10, 2025 11:07
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, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@dmarek-flex dmarek-flex 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 to me

@daquinteroflex daquinteroflex force-pushed the dario/fix_rf_license_warning branch from a31e79f to db19209 Compare December 10, 2025 17:55
@daquinteroflex daquinteroflex force-pushed the dario/fix_rf_license_warning branch from db19209 to 087a7e3 Compare December 10, 2025 17:55
@daquinteroflex daquinteroflex added this pull request to the merge queue Dec 10, 2025
@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/components/simulation.py (100%)
  • tidy3d/log.py (100%)
  • tidy3d/plugins/smatrix/component_modelers/base.py (100%)

Summary

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

Merged via the queue into develop with commit 8fab9ee Dec 10, 2025
47 checks passed
@daquinteroflex daquinteroflex deleted the dario/fix_rf_license_warning branch December 10, 2025 18:53
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