Skip to content

Conversation

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Nov 5, 2025

  • add tests

Greptile Overview

Updated On: 2025-11-05 20:52:36 UTC

Greptile Summary

This PR introduces BroadbandPulse, a new source time dependence class that enables exciting simulations across a wide frequency spectrum. The feature is implemented as a wrapper around the tidy3d-extras licensed implementation.

Key changes:

  • Added BroadbandPulse class in tidy3d/components/source/time.py with configurable frequency range and minimum amplitude
  • Refactored tidy3d-extras checking logic into reusable helper functions (_check_tidy3d_extras_available and check_tidy3d_extras_licensed_feature)
  • Updated documentation and CHANGELOG to reflect the new feature

Issues found:

  • Syntax error in tidy3d/packaging.py:244 - missing 'f' prefix on f-string causes literal {exc!s} to appear in error message instead of being interpolated

Confidence Score: 4/5

  • This PR is mostly safe to merge with one syntax error that needs fixing
  • The implementation is well-structured with proper licensing checks and documentation. However, there's a syntax error in the error message formatting that will cause incorrect error messages to be displayed to users when tidy3d-extras fails to load. This is a user-facing bug that should be fixed before merge.
  • Pay attention to tidy3d/packaging.py which contains a syntax error on line 244

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/source/time.py 4/5 Added BroadbandPulse class that wraps tidy3d-extras implementation for wide frequency spectrum excitation
tidy3d/packaging.py 2/5 Refactored tidy3d-extras checking logic with new helper functions; contains f-string formatting bug on line 244

Sequence Diagram

sequenceDiagram
    participant User
    participant BroadbandPulse
    participant Validator
    participant PackagingModule
    participant TidyExtras

    User->>BroadbandPulse: Instantiate with freq_range, minimum_amplitude
    BroadbandPulse->>Validator: _check_broadband_pulse_available()
    Validator->>PackagingModule: check_tidy3d_extras_licensed_feature("BroadbandPulse")
    PackagingModule->>PackagingModule: _check_tidy3d_extras_available()
    
    alt tidy3d-extras not loaded
        PackagingModule->>TidyExtras: import tidy3d_extras
        alt Import fails
            TidyExtras-->>PackagingModule: ImportError
            PackagingModule-->>Validator: Tidy3dImportError
            Validator-->>User: Error: Install tidy3d-extras
        else Import succeeds
            TidyExtras-->>PackagingModule: tidy3d_extras_mod
            PackagingModule->>PackagingModule: Check version and features
            alt BroadbandPulse not in features
                PackagingModule-->>Validator: Tidy3dImportError
                Validator-->>User: Error: Feature not licensed
            else Feature available
                PackagingModule->>PackagingModule: Cache tidy3d_extras["mod"]
                Validator-->>BroadbandPulse: Validation passed
            end
        end
    else tidy3d-extras already loaded
        PackagingModule->>PackagingModule: Check features list
        alt BroadbandPulse not in features
            PackagingModule-->>Validator: Tidy3dImportError
            Validator-->>User: Error: Feature not licensed
        else Feature available
            Validator-->>BroadbandPulse: Validation passed
        end
    end
    
    BroadbandPulse->>User: Instance created
    
    User->>BroadbandPulse: Call amp_time(time) or amp_freq(freq)
    BroadbandPulse->>BroadbandPulse: Access _source property
    BroadbandPulse->>TidyExtras: extension.BroadbandPulse(...)
    TidyExtras-->>BroadbandPulse: BroadbandPulse instance
    BroadbandPulse->>TidyExtras: _source.amp_time(time)
    TidyExtras-->>BroadbandPulse: Complex amplitude
    BroadbandPulse-->>User: Return amplitude
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.

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-2556-microwave branch 2 times, most recently from 6c05805 to 026198a Compare November 5, 2025 20:53
@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-2556-microwave branch from 026198a to b00febc Compare November 5, 2025 22:19
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

Diff Coverage

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

  • tidy3d/components/source/time.py (57.6%): Missing lines 636-639,642,647-648,653,664,668,672,676,680,685
  • tidy3d/packaging.py (50.0%): Missing lines 207,209-211,216-217,224,241-244,246-248

Summary

  • Total: 61 lines
  • Missing: 28 lines
  • Coverage: 54%

tidy3d/components/source/time.py

  632 
  633     @pydantic.validator("freq_range", always=True)
  634     def _validate_freq_range(cls, val):
  635         """Validate that freq_range is positive and properly ordered."""
! 636         if val[0] <= 0 or val[1] <= 0:
! 637             raise ValidationError("Both elements of 'freq_range' must be positive.")
! 638         if val[1] <= val[0]:
! 639             raise ValidationError(
  640                 f"'freq_range[1]' ({val[1]}) must be greater than 'freq_range[0]' ({val[0]})."
  641             )
! 642         return val
  643 
  644     @pydantic.root_validator()
  645     def _check_broadband_pulse_available(cls, values):
  646         """Check if BroadbandPulse is available."""
! 647         check_tidy3d_extras_licensed_feature("BroadbandPulse")
! 648         return values
  649 
  650     @cached_property
  651     def _source(self):
  652         """Implementation of broadband pulse."""
! 653         return tidy3d_extras["mod"].extension.BroadbandPulse(
  654             fmin=self.freq_range[0],
  655             fmax=self.freq_range[1],
  656             minRelAmp=self.minimum_amplitude,
  657             amp=self.amplitude,

  660         )
  661 
  662     def end_time(self) -> float:
  663         """Time after which the source is effectively turned off / close to zero amplitude."""
! 664         return self._source.end_time(END_TIME_FACTOR_GAUSSIAN)
  665 
  666     def amp_time(self, time: float) -> complex:
  667         """Complex-valued source amplitude as a function of time."""
! 668         return self._source.amp_time(time)
  669 
  670     def amp_freq(self, freq: float) -> complex:
  671         """Complex-valued source amplitude as a function of frequency."""
! 672         return self._source.amp_freq(freq)
  673 
  674     def frequency_range_sigma(self, sigma: float = DEFAULT_SIGMA) -> FreqBound:
  675         """Frequency range where the source amplitude is within ``exp(-sigma**2/2)`` of the peak amplitude."""
! 676         return self._source.frequency_range(sigma)
  677 
  678     def frequency_range(self, num_fwidth: float = DEFAULT_SIGMA) -> FreqBound:
  679         """Frequency range where the source amplitude is within ``exp(-sigma**2/2)`` of the peak amplitude."""
! 680         return self.frequency_range_sigma(num_fwidth)
  681 
  682     @cached_property
  683     def _freq0(self) -> float:
  684         """Central frequency from frequency range."""
! 685         return np.mean(self.freq_range)
  686 
  687 
  688 SourceTimeType = Union[GaussianPulse, ContinuousWave, CustomSourceTime, BroadbandPulse]

tidy3d/packaging.py

  203                 "example, 'pip install tidy3d[extras]'."
  204             ) from exc
  205 
  206         else:
! 207             version = tidy3d_extras_mod.__version__
  208 
! 209             if version is None:
! 210                 tidy3d_extras["mod"] = None
! 211                 raise Tidy3dImportError(
  212                     "The package 'tidy3d-extras' did not initialize correctly, "
  213                     "likely due to an invalid API key."
  214                 )
  215 
! 216             if version != __version__:
! 217                 log.warning(
  218                     "The package 'tidy3d-extras' is required for this "
  219                     "operation. The version of 'tidy3d-extras' should match "
  220                     "the version of 'tidy3d'. You can install the correct "
  221                     "version using 'pip install tidy3d[extras]'."

  220                     "the version of 'tidy3d'. You can install the correct "
  221                     "version using 'pip install tidy3d[extras]'."
  222                 )
  223 
! 224             tidy3d_extras["mod"] = tidy3d_extras_mod
  225 
  226 
  227 def check_tidy3d_extras_licensed_feature(feature_name: str):
  228     """Helper function to check if a specific feature is licensed in 'tidy3d-extras'.

  237     Tidy3dImportError
  238         If the feature is not available with your license.
  239     """
  240 
! 241     try:
! 242         _check_tidy3d_extras_available()
! 243     except Tidy3dImportError as exc:
! 244         raise Tidy3dImportError(f"Failed to load 'tidy3d-extras'. {exc!s}") from exc
  245     else:
! 246         features = tidy3d_extras["mod"].extension._features()
! 247         if feature_name not in features:
! 248             raise Tidy3dImportError(
  249                 f"The feature '{feature_name}' is not available with your license. "
  250                 "Please contact Tidy3D support, or upgrade your license."
  251             )

tidy3d_extras["mod"] = tidy3d_extras_mod
tidy3d_extras["use_local_subpixel"] = "local_subpixel" in features
try:
_check_tidy3d_extras_available()
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, you need to set tidy3d_extras["use_local_subpixel"] = True, right? The backend tidy3d-extras tests should catch this?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, can you just make this use your check_tidy3d_extras_licensed_feature function too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In your current code, it doesn't raise an error if the subpixel is not licensed: it just silently disables local subpixel. Do you think we should instead raise an error here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(the error is raised in the revised version)

5.0,
title="Offset",
description="Time delay of the maximum value of the "
"pulse in units of 1 / (``2pi * fwidth``).",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this was copy-pasted from Pulse, but BroadbandPulse doesn't have an fwidth?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, that's the part I have been working on since yesterday: offset is not well defined in this pulse. I need to find a better definition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 678 to 680
def frequency_range(self, num_fwidth: float = DEFAULT_SIGMA) -> FreqBound:
"""Frequency range where the source amplitude is within ``exp(-sigma**2/2)`` of the peak amplitude."""
return self.frequency_range_sigma(num_fwidth)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This just delegates to frequency_range_sigma but it keeps the sigma-based docstring, which is misleading. This should provide the correct description for the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now it's "Delegated to frequency_range_sigma(sigma=num_fwidth) for computing the frequency range where the source amplitude is within exp(-num_fwidth**2/2) of the peak amplitude"

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-2556-microwave branch from 72a3ed2 to dc16fac Compare November 8, 2025 05:44
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