Skip to content

Conversation

@momchil-flex
Copy link
Collaborator

@momchil-flex momchil-flex commented Dec 5, 2025

Tom noticed that outer_dot would always return the overlap data with frequency coords that are sorted in increasing order, making them mismatch the original data if it e.g. had them in decreasing order. This fixes that.

Greptile Overview

Greptile Summary

This PR fixes a bug in the outer_dot method where frequency coordinates were being sorted in increasing order, breaking alignment when the original data had frequencies in a different order (e.g., decreasing). The fix replaces the old implementation that used sorted(set(...).intersection(...)) with pandas Index.intersection(sort=False) to preserve the frequency order from the first dataset.

Key changes:

  • Replaced frequency intersection logic with pandas Index operations that preserve order
  • Added get_indexer to properly map frequencies between datasets
  • Added comprehensive test coverage for reversed frequency ordering
  • Cleaned up unused exception variables in tests (as e removed where not used)

Confidence Score: 5/5

  • This PR is safe to merge with no concerns
  • The fix is minimal, well-tested, and uses the correct pandas API to preserve frequency order. The implementation correctly replaces the problematic sorted intersection with order-preserving Index operations. Comprehensive test coverage validates both the intersection size and order preservation with reversed frequencies.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/data/monitor_data.py 5/5 Replaced sorted set intersection with pandas Index.intersection(sort=False) to preserve frequency order from first dataset
tests/test_data/test_monitor_data.py 5/5 Added test case for reversed frequency order and removed unused exception variables in test assertions
CHANGELOG.md 5/5 Added entry under Fixed section describing the frequency order bug fix in outer_dot

Sequence Diagram

sequenceDiagram
    participant Client as Client Code
    participant Self as ElectromagneticFieldData (self)
    participant Other as ElectromagneticFieldData (other)
    participant PI as pandas.Index
    
    Client->>Self: outer_dot(field_data)
    Self->>Self: Get tangential fields from self
    Self->>Other: _interpolated_tangential_fields()
    Other-->>Self: Return interpolated fields
    
    Self->>Self: Extract frequency coords from both datasets
    Self->>PI: Index(coords[0]["f"].values)
    PI-->>Self: freq_self Index
    Self->>PI: Index(coords[1]["f"].values)
    PI-->>Self: freq_other Index
    
    Self->>PI: freq_self.intersection(freq_other, sort=False)
    Note over PI: Preserves order from freq_self<br/>(unlike old sorted approach)
    PI-->>Self: common_freqs Index
    
    Self->>PI: freq_self.get_indexer(common_freqs)
    PI-->>Self: isel1 (indices for self)
    Self->>PI: freq_other.get_indexer(common_freqs)
    PI-->>Self: isel2 (indices for other)
    
    Self->>Self: Apply isel1 to self fields
    Self->>Other: Apply isel2 to other fields
    
    Self->>Self: Compute dot product with aligned frequencies
    Self-->>Client: Return result with original frequency order
Loading

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug in the outer_dot method where output frequencies were always sorted in increasing order, causing them to mismatch the original input data if it had frequencies in a different order (e.g., decreasing).

Key Changes:

  • Modified outer_dot to use pandas Index.intersection(sort=False) and get_indexer to preserve frequency order from the first dataset
  • Added comprehensive tests to verify frequency order is maintained
  • Cleaned up unused exception variables in test assertions

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tidy3d/components/data/monitor_data.py Replaced sorted set-based frequency intersection with pandas Index operations to preserve frequency order
tests/test_data/test_monitor_data.py Added tests for frequency order preservation and removed unused exception variables from pytest.raises statements
CHANGELOG.md Documented the bug fix for frequency ordering in outer_dot

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

Diff Coverage

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

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

Summary

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

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