Skip to content

Conversation

weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Oct 9, 2025

  • add tests

Greptile Overview

Updated On: 2025-10-09 23:26:28 UTC

Greptile Summary

This PR introduces support for the tidy3d-microwave plugin and adds a new BroadbandPulse source time class for microwave simulations. The changes implement a modular architecture that allows Tidy3D to conditionally support microwave-specific functionality through an optional external package.

The implementation follows the established pattern used for other optional dependencies like tidy3d-extras. Key components include: packaging infrastructure with a @supports_microwave decorator that handles import validation and version checking; a BroadbandPulse class that acts as a bridge between Tidy3D's source time interface and the microwave package's implementation; build configuration updates in pyproject.toml; API documentation additions; and basic integration tests.

The BroadbandPulse class enables users to create broadband excitation sources with guaranteed amplitude levels across specified frequency ranges, particularly useful for microwave simulation applications. The modular design ensures users can access this functionality by installing the optional tidy3d[microwave] package while keeping the core Tidy3D package lightweight.

PR Description Notes:

  • The PR body only contains "- [ ] add tests" which suggests this is a work-in-progress checklist item

Important Files Changed

Changed Files
Filename Score Overview
CHANGELOG.md 3/5 Added changelog entries for tidy3d-microwave plugin and BroadbandPulse feature with minor spelling/grammar issues from previous reviews
tidy3d/packaging.py 4/5 Added microwave package support infrastructure with decorator and version checking following established patterns
docs/api/sources.rst 5/5 Added BroadbandPulse to source time dependencies documentation
tidy3d/components/source/time.py 3/5 Implemented BroadbandPulse class but contains magic numbers in validation constraints that need addressing
tests/test_components/test_packaging.py 3/5 Added basic integration tests but has limited coverage and unused variable issue from previous review
pyproject.toml 5/5 Added microwave extras group configuration following existing patterns

Confidence score: 3/5

  • This PR introduces well-architected functionality but has implementation issues that need attention before merging
  • Score reflects unresolved issues from previous reviews, limited test coverage, magic numbers in validation, and dependency on potentially unavailable external package
  • Pay close attention to the BroadbandPulse implementation validation constraints and test coverage for error handling scenarios

Sequence Diagram

sequenceDiagram
    participant User
    participant BroadbandPulse as "BroadbandPulse"
    participant tidy3d_microwave as "tidy3d_microwave module"
    participant Packaging as "packaging.py"
    participant SourceImplementation as "tidy3d_microwave.BroadbandPulse"

    User->>BroadbandPulse: "Initialize BroadbandPulse(freq_range, minimum_amplitude, offset)"
    BroadbandPulse->>Packaging: "@supports_microwave decorator called"
    Packaging->>tidy3d_microwave: "Check if tidy3d_microwave module available"
    
    alt tidy3d_microwave available
        tidy3d_microwave-->>Packaging: "Module found and validated"
        Packaging-->>BroadbandPulse: "Proceed with initialization"
        BroadbandPulse->>SourceImplementation: "Create tidy3d_microwave.BroadbandPulse(fmin, fmax, minRelAmp, amp, phase, offset)"
        SourceImplementation-->>BroadbandPulse: "Return implementation instance"
        BroadbandPulse-->>User: "BroadbandPulse instance created"
    else tidy3d_microwave not available
        tidy3d_microwave-->>Packaging: "ImportError raised"
        Packaging-->>BroadbandPulse: "Tidy3dImportError: install tidy3d[microwave]"
        BroadbandPulse-->>User: "Error: Missing tidy3d-microwave package"
    end

    User->>BroadbandPulse: "Call amp_time(time)"
    BroadbandPulse->>SourceImplementation: "Delegate to _source.amp_time(time)"
    SourceImplementation-->>BroadbandPulse: "Return complex amplitude"
    BroadbandPulse-->>User: "Complex amplitude value"

    User->>BroadbandPulse: "Call amp_freq(freq)"
    BroadbandPulse->>SourceImplementation: "Delegate to _source.amp_freq(freq)"
    SourceImplementation-->>BroadbandPulse: "Return complex amplitude"
    BroadbandPulse-->>User: "Complex amplitude value"

    User->>BroadbandPulse: "Call end_time()"
    BroadbandPulse->>SourceImplementation: "Delegate to _source.end_time(END_TIME_FACTOR_GAUSSIAN)"
    SourceImplementation-->>BroadbandPulse: "Return end time"
    BroadbandPulse-->>User: "End time value"

    User->>BroadbandPulse: "Call frequency_range_sigma(sigma)"
    BroadbandPulse->>SourceImplementation: "Delegate to _source.frequency_range(sigma)"
    SourceImplementation-->>BroadbandPulse: "Return frequency bounds"
    BroadbandPulse-->>User: "Frequency range tuple"
Loading

Context used (3)

  • Rule from dashboard - Avoid hardcoding values ("magic numbers") that can be programmatically derived from data; use named ... (source)
  • Rule from dashboard - In changelogs, enclose code identifiers (class, function names) in backticks and use specific names ... (source)
  • Rule from dashboard - Require a changelog entry for any PR that is not purely an internal refactor. (source)

@weiliangjin2021 weiliangjin2021 marked this pull request as draft October 9, 2025 23:09
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.

6 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-2556-microwave branch from fd3c06b to 84a91a5 Compare October 9, 2025 23:18
@weiliangjin2021
Copy link
Collaborator Author

@greptile

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.

6 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +95 to +111
def test_tidy3d_microwave():
import importlib

has_tidy3d_microwave = importlib.util.find_spec("tidy3d_microwave") is not None
print(f"has_tidy3d_microwave = {has_tidy3d_microwave}")

if has_tidy3d_microwave:

@supports_microwave
def get_pulse():
assert tidy3d_microwave["mod"] is not None
pulse = tidy3d_microwave["mod"].BroadbandPulse(fmin=0.1, fmax=10)
_ = pulse.frequency_range(2)

get_pulse()
else:
assert tidy3d_microwave["mod"] is None
Copy link

Choose a reason for hiding this comment

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

style: Test only covers the success path. Consider adding a test case that verifies Tidy3dImportError is raised when @supports_microwave decorated functions are called without the package installed, similar to other tests in this file.

Context Used: Rule from dashboard - Add tests for all public constructors and methods to confirm expected behavior, including effects on... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/test_packaging.py
Line: 95:111

Comment:
**style:** Test only covers the success path. Consider adding a test case that verifies `Tidy3dImportError` is raised when `@supports_microwave` decorated functions are called without the package installed, similar to other tests in this file.

**Context Used:** Rule from `dashboard` - Add tests for all public constructors and methods to confirm expected behavior, including effects on... ([source](https://app.greptile.com/review/custom-context?memory=40c78813-75a7-47ea-a080-1509e6e686a9))

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Contributor

github-actions bot commented Oct 9, 2025

Diff Coverage

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

  • tidy3d/components/source/time.py (68.2%): Missing lines 132,143,147,151,155,159,164
  • tidy3d/packaging.py (26.3%): Missing lines 262-264,266-268,276,278-280,285-286,293,295

Summary

  • Total: 41 lines
  • Missing: 21 lines
  • Coverage: 48%

tidy3d/components/source/time.py

  128     @cached_property
  129     @supports_microwave
  130     def _source(self):
  131         """Implementation of broadband pulse."""
! 132         return tidy3d_microwave["mod"].BroadbandPulse(
  133             fmin=self.freq_range[0],
  134             fmax=self.freq_range[1],
  135             minRelAmp=self.minimum_amplitude,
  136             amp=self.amplitude,

  139         )
  140 
  141     def end_time(self) -> float:
  142         """Time after which the source is effectively turned off / close to zero amplitude."""
! 143         return self._source.end_time(END_TIME_FACTOR_GAUSSIAN)
  144 
  145     def amp_time(self, time: float) -> complex:
  146         """Complex-valued source amplitude as a function of time."""
! 147         return self._source.amp_time(time)
  148 
  149     def amp_freq(self, freq: float) -> complex:
  150         """Complex-valued source amplitude as a function of frequency."""
! 151         return self._source.amp_freq(freq)
  152 
  153     def frequency_range_sigma(self, sigma: float = DEFAULT_SIGMA) -> FreqBound:
  154         """Frequency range where the source amplitude is within ``exp(-sigma**2/2)`` of the peak amplitude."""
! 155         return self._source.frequency_range(sigma)
  156 
  157     def frequency_range(self, num_fwidth: float = DEFAULT_SIGMA) -> FreqBound:
  158         """Frequency range where the source amplitude is within ``exp(-sigma**2/2)`` of the peak amplitude."""
! 159         return self.frequency_range_sigma(num_fwidth)
  160 
  161     @cached_property
  162     def _freq0(self) -> float:
  163         """Central frequency from frequency range."""
! 164         return np.mean(self.freq_range)
  165 
  166 
  167 class Pulse(SourceTime, ABC):
  168     """A source time that ramps up with some ``fwidth`` and oscillates at ``freq0``."""

tidy3d/packaging.py

  258 
  259     @functools.wraps(fn)
  260     def _fn(*args, **kwargs):
  261         # first try to import the module
! 262         if tidy3d_microwave["mod"] is None:
! 263             try:
! 264                 import tidy3d_microwave as tidy3d_microwave_mod
  265 
! 266             except ImportError as exc:
! 267                 tidy3d_microwave["mod"] = None
! 268                 raise Tidy3dImportError(
  269                     "The package 'tidy3d-microwave' is required for this "
  270                     "operation. "
  271                     "Please install the 'tidy3d-microwave' package using, for "
  272                     "example, 'pip install tidy3d[microwave]'."

  272                     "example, 'pip install tidy3d[microwave]'."
  273                 ) from exc
  274 
  275             else:
! 276                 version = tidy3d_microwave_mod.__version__
  277 
! 278                 if version is None:
! 279                     tidy3d_microwave["mod"] = None
! 280                     raise Tidy3dImportError(
  281                         "The package 'tidy3d-microwave' did not initialize correctly, "
  282                         "likely due to an invalid API key."
  283                     )
  284 
! 285                 if version != __version__:
! 286                     log.warning(
  287                         "The package 'tidy3d-microwave' is required for this "
  288                         "operation. The version of 'tidy3d-microwave' should match "
  289                         "the version of 'tidy3d'. You can install the correct "
  290                         "version using 'pip install tidy3d[microwave]'."

  289                         "the version of 'tidy3d'. You can install the correct "
  290                         "version using 'pip install tidy3d[microwave]'."
  291                     )
  292 
! 293                 tidy3d_microwave["mod"] = tidy3d_microwave_mod
  294 
! 295         return fn(*args, **kwargs)
  296 
  297     return _fn

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.

1 participant