Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Oct 31, 2025

Greptile Overview

Updated On: 2025-10-31 16:22:57 UTC

Greptile Summary

Fixes frequency sampling in MicrowaveModeData when group index calculation is enabled. Group index calculation requires triplet frequencies (backward, center, forward) for numerical differentiation, but the TransmissionLineDataset was incorrectly retaining all triplet frequencies instead of filtering back to the original center frequencies.

Key Changes:

  • Extracted _group_index_freq_slices() helper method in ModeData to centralize frequency slice logic
  • Overrode _group_index_post_process() in MicrowaveModeData to filter transmission_line_data frequencies
  • Added comprehensive test test_mode_solver_with_microwave_group_index() to verify frequency filtering

Issues Found:

  • Missing None check for transmission_line_data before accessing attributes (could cause AttributeError)
  • Minor grammar issue in CHANGELOG entry ("to when" → "when")

Confidence Score: 3/5

  • This PR fixes a valid bug but has a critical logic error that could cause runtime failures
  • The fix correctly addresses the frequency sampling issue by filtering transmission line data to match the original frequencies after group index calculation. However, the implementation in _group_index_post_process() accesses transmission_line_data attributes without checking if it's None first (it's declared as Optional[TransmissionLineDataset]), which will cause an AttributeError in cases where transmission line data is not populated
  • Pay close attention to tidy3d/components/microwave/data/monitor_data.py - the _group_index_post_process() method needs a None check before accessing transmission_line_data

Important Files Changed

File Analysis

Filename Score Overview
CHANGELOG.md 4/5 Added changelog entry with minor grammar issue
tidy3d/components/data/monitor_data.py 5/5 Extracted frequency slice logic into reusable helper method
tidy3d/components/microwave/data/monitor_data.py 3/5 Overrides group index post-processing to filter transmission line frequencies, but lacks None check
tests/test_components/test_microwave.py 5/5 Comprehensive test validates frequency filtering in transmission line data during group index calculation

Sequence Diagram

sequenceDiagram
    participant MS as ModeSolver
    participant MSD as MicrowaveModeSolverData
    participant MMD as MicrowaveModeData
    participant MD as ModeData
    
    MS->>MS: _get_data_with_group_index()
    Note over MS: Expands frequencies to triplets<br/>(back, center, forward) for<br/>numerical differentiation
    MS->>MS: _freqs_for_group_index()
    Note over MS: Creates [f1*(1-δ), f1, f1*(1+δ),<br/>f2*(1-δ), f2, f2*(1+δ), ...]
    MS->>MSD: Solve with expanded frequencies
    Note over MSD: data_raw contains triplet frequencies<br/>in transmission_line_data
    MS->>MMD: _group_index_post_process(frequency_step)
    MMD->>MD: super()._group_index_post_process()
    MD->>MD: _group_index_freq_slices()
    Note over MD: Returns (back, center, fwd) slices<br/>slice(0, n, 3), slice(1, n, 3), slice(2, n, 3)
    MD->>MD: Calculate n_group from triplets
    MD->>MD: Filter frequencies to center values
    MD-->>MMD: Returns filtered ModeData
    MMD->>MMD: _group_index_freq_slices()
    Note over MMD: Reuse same frequency slices
    MMD->>MMD: Filter transmission_line_data
    Note over MMD: Z0.isel(f=center_inds)<br/>voltage_coeffs.isel(f=center_inds)<br/>current_coeffs.isel(f=center_inds)
    MMD->>MMD: updated_copy(**update_dict)
    MMD-->>MS: Returns MicrowaveModeData<br/>with original frequencies only
Loading

@dmarek-flex dmarek-flex self-assigned this Oct 31, 2025
@dmarek-flex dmarek-flex added the RF label Oct 31, 2025
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, 2 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

Diff Coverage

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

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

Summary

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

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-3939-fix-microwave-mode-data-with-group-index-calc branch 2 times, most recently from f7e3677 to e301cec Compare October 31, 2025 17:15
@dmarek-flex dmarek-flex added the rc3 3rd pre-release label Oct 31, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-3939-fix-microwave-mode-data-with-group-index-calc branch from e301cec to 610e117 Compare November 3, 2025 22:03
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. Thanks for the fix!

@dmarek-flex dmarek-flex added this pull request to the merge queue Nov 4, 2025
Merged via the queue into develop with commit 8da1ce8 Nov 4, 2025
37 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/FXC-3939-fix-microwave-mode-data-with-group-index-calc branch November 4, 2025 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rc3 3rd pre-release RF

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants