Skip to content

Conversation

@dbochkov-flexcompute
Copy link
Contributor

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

This is a little bit silly: while MicrowaveModeSolverData is inherited ModeSolverData, which contains the correct _stored_freqs, it also is inherited from MicrowaveModeData, which takes the priority and which is, in its turn, inherited from ModeData, with _stored_freqs incompatible with ModeSolverData/MicrowaveModeSolverData.

Greptile Overview

Greptile Summary

Fixes a Python Method Resolution Order (MRO) bug in MicrowaveModeSolverMonitor where the _stored_freqs property was returning incorrect values due to inheritance priority.

The Issue:

  • MicrowaveModeSolverMonitor inherits from both MicrowaveModeMonitor (which inherits ModeMonitor) and ModeSolverMonitor
  • Python's MRO gives priority to MicrowaveModeMonitor, which inherits _stored_freqs from ModeMonitor (returns all frequencies)
  • But ModeSolverMonitor._stored_freqs correctly handles frequency reduction with interp_spec by calling mode_spec._sampling_freqs_mode_solver_data
  • Result: MicrowaveModeSolverMonitor was using the wrong implementation that ignored interp_spec settings

The Fix:

  • Added explicit _stored_freqs property override in MicrowaveModeSolverMonitor that calls mode_spec._sampling_freqs_mode_solver_data
  • Added comprehensive parametrized test covering various scenarios with/without interp_spec and reduce_data flags
  • Test validates both regular ModeSolverMonitor and MicrowaveModeSolverMonitor behavior (via rf parameter)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The fix is surgical and correct - it adds a 3-line property override that resolves a specific MRO inheritance bug. The implementation mirrors the existing ModeSolverMonitor._stored_freqs logic exactly. Comprehensive test coverage verifies the fix across multiple scenarios including edge cases.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/microwave/monitor.py 5/5 Added _stored_freqs property override to fix MRO inheritance issue with mode solver data storage
tests/test_components/test_mode_interp.py 5/5 Added comprehensive parametrized test to verify _stored_freqs behavior for both regular and microwave mode solver monitors

Sequence Diagram

sequenceDiagram
    participant User
    participant MicrowaveModeSolverMonitor
    participant MicrowaveModeMonitor
    participant ModeMonitor
    participant ModeSolverMonitor
    
    Note over MicrowaveModeSolverMonitor: Class hierarchy: MicrowaveModeSolverMonitor(MicrowaveModeMonitor, ModeSolverMonitor)
    Note over MicrowaveModeMonitor: MicrowaveModeMonitor inherits from ModeMonitor
    Note over ModeMonitor: ModeMonitor._stored_freqs returns self.freqs (all frequencies)
    Note over ModeSolverMonitor: ModeSolverMonitor._stored_freqs calls mode_spec._sampling_freqs_mode_solver_data (reduced frequencies with interp_spec)
    
    User->>MicrowaveModeSolverMonitor: Access _stored_freqs property
    
    rect rgb(255, 200, 200)
        Note over MicrowaveModeSolverMonitor: BEFORE FIX: MRO looks up MicrowaveModeMonitor first
        MicrowaveModeSolverMonitor->>MicrowaveModeMonitor: Check for _stored_freqs
        MicrowaveModeMonitor->>ModeMonitor: Inherit _stored_freqs
        ModeMonitor-->>MicrowaveModeSolverMonitor: Returns self.freqs (WRONG - ignores interp_spec)
    end
    
    rect rgb(200, 255, 200)
        Note over MicrowaveModeSolverMonitor: AFTER FIX: Override _stored_freqs directly
        MicrowaveModeSolverMonitor->>MicrowaveModeSolverMonitor: Use own _stored_freqs
        MicrowaveModeSolverMonitor->>MicrowaveModeSolverMonitor: Call mode_spec._sampling_freqs_mode_solver_data
        MicrowaveModeSolverMonitor-->>User: Returns reduced frequencies (CORRECT - respects interp_spec)
    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.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@weiliangjin2021
Copy link
Collaborator

Does storage_size need to be overridden too? If so, how about switching the inheritance order: class MicrowaveModeSolverMonitor(MicrowaveModeMonitor, ModeSolverMonitor) -> class MicrowaveModeSolverMonitor(ModeSolverMonitor, MicrowaveModeMonitor)

@dmarek-flex
Copy link
Contributor

dmarek-flex commented Nov 17, 2025

I guess I knew this would start causing issues. The complicated inheritance is not great. I was trying an approach like @weiliangjin2021 suggested, but I think a few more changes are required. One of them is that mode_spec will have the wrong type in the new order.

I think an approach that might work better is to break this diamond inheritance and use a mixin pattern. Then the MicrowaveModeData and MicrowaveModeSolverData inherit from the associated photonics version but also the mixin which provides the overrides for manipulating the RF-specific data. Could also use a mixin for the monitors that provides the correct mode_spec type, perhaps overkill at this point but reduces code duplication.

I'll try to make a quick PR that you can include with your test.

@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/components/microwave/data/monitor_data.py (100%)
  • tidy3d/components/microwave/monitor.py (100%)

Summary

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

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.

Think this is good, you can drop my commit after rebasing though

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!

@dbochkov-flexcompute
Copy link
Contributor Author

Think this is good, you can drop my commit after rebasing though

thanks for the neat refactor @dmarek-flex !

@dbochkov-flexcompute dbochkov-flexcompute added this pull request to the merge queue Nov 17, 2025
Merged via the queue into develop with commit 2477d96 Nov 17, 2025
70 checks passed
@dbochkov-flexcompute dbochkov-flexcompute deleted the hotfix/microwave-mode-downsampling branch November 17, 2025 22:11
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