Skip to content

Conversation

weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Oct 7, 2025

Aside from the issue described in https://flow360.atlassian.net/browse/FXC-3541, this is also a preparation for https://flow360.atlassian.net/browse/FXC-2556

Greptile Overview

Updated On: 2025-10-07 23:42:48 UTC

Summary

This PR implements significant improvements to frequency range calculations and central frequency determination for source time dependencies, particularly for GaussianPulse sources when DC component removal is enabled. The core change introduces a new `frequency_range_sigma()` method that uses transcendental equation solving to find precise frequency bounds where amplitude drops to exp(-σ²/2) of the peak, replacing the less accurate standard deviation-based `frequency_range()` method.

The PR systematically refactors the codebase to use a private _freq0 property instead of the public freq0 property throughout the simulation framework. This new property provides more accurate central frequency calculations by intelligently choosing between user-specified freq0 (for Pulse sources) and computed _freq0_sigma_centroid (for other source types). This ensures consistent frequency handling across different source time types.

Additionally, the PR enhances the testing infrastructure with new log assertion utilities (AssertLogStr and assert_str_in_log) that enable more granular validation of log message content beyond just checking log levels. The changes also add a new pyroots dependency for numerical root-finding operations required by the improved frequency calculations.

These modifications address accuracy issues described in FXC-3541 and prepare the codebase for future enhancements mentioned in FXC-2556, ensuring that electromagnetic simulations have more precise frequency domain representations, especially for asymmetric spectral content like DC-removed Gaussian pulses.

Important Files Changed

Changed Files
Filename Score Overview
tidy3d/components/source/time.py 4/5 Core implementation of improved frequency range calculations with new frequency_range_sigma() method and enhanced central frequency determination
tidy3d/components/simulation.py 4/5 Systematic replacement of freq0 with _freq0 across 14 locations for more accurate frequency calculations
tidy3d/components/data/sim_data.py 4/5 Updates adjoint source processing to use private _freq0 property for consistent frequency handling
tests/utils.py 4/5 Adds new log assertion utilities with AssertLogStr and assert_str_in_log for enhanced test validation
tests/test_components/test_source.py 4/5 Adds comprehensive tests for new frequency_range_sigma() method and validates improved frequency calculations
tests/test_components/test_simulation.py 4/5 Updates test assertions to use more precise string-based log validation instead of just log levels
tidy3d/components/source/base.py 4/5 Modifies frequency validation to use _freq0_sigma_centroid for more accurate minimum frequency checks
tidy3d/components/source/field.py 4/5 Updates broadband sources to use frequency_range_sigma() and _freq0 for improved accuracy
tidy3d/components/boundary.py 5/5 Simple but important change from freq0 to _freq0 in boundary condition frequency calculations
tidy3d/components/grid/grid_spec.py 4/5 Updates grid generation to use _freq0 for more accurate wavelength calculations
tidy3d/plugins/smatrix/ports/coaxial_lumped.py 5/5 Updates coaxial port source creation to use computed central frequency for better consistency
tests/test_components/autograd/test_autograd.py 5/5 Fixes DataArray creation bug by converting map object to tuple for dimension names
CHANGELOG.md 5/5 Documents the frequency range accuracy improvement for GaussianPulse sources

Confidence score: 4/5

  • This PR introduces substantial improvements to frequency calculations with comprehensive testing, but the complexity of the changes requires careful validation
  • Score reflects the thorough implementation across multiple files and good test coverage, but lowered due to the mathematical complexity and potential edge cases in transcendental equation solving
  • Pay close attention to tidy3d/components/source/time.py and tests/utils.py for the core frequency calculation logic and debug statements that should be removed

Context used:

Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
Rule from dashboard - Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)

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.

Additional Comments (1)

  1. tests/utils.py, line 1557 (link)

    style: Remove debug statement before merging

    Context Used: Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)

13 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

github-actions bot commented Oct 8, 2025

Diff Coverage

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

  • tidy3d/components/boundary.py (100%)
  • tidy3d/components/data/sim_data.py (100%)
  • tidy3d/components/grid/grid_spec.py (100%)
  • tidy3d/components/grid/mesher.py (100%)
  • tidy3d/components/simulation.py (100%)
  • tidy3d/components/source/base.py (100%)
  • tidy3d/components/source/field.py (44.4%): Missing lines 108,519-521,525
  • tidy3d/components/source/time.py (94.3%): Missing lines 95,232,241,254,265

Summary

  • Total: 121 lines
  • Missing: 10 lines
  • Coverage: 91%

tidy3d/components/source/field.py

  104     @cached_property
  105     def frequency_grid(self) -> np.ndarray:
  106         """A Chebyshev grid used to approximate frequency dependence."""
  107         if self.num_freqs == 1:
! 108             return np.array([self.source_time._freq0])
  109         freq_min, freq_max = self.source_time.frequency_range_sigma(sigma=CHEB_GRID_WIDTH)
  110         return self._chebyshev_freq_grid(freq_min, freq_max)
  111 
  112     def _chebyshev_freq_grid(self, freq_min, freq_max):

  515 
  516     @cached_property
  517     def frequency_grid(self) -> np.ndarray:
  518         """A Chebyshev grid used to approximate frequency dependence."""
! 519         if self.num_freqs == 1:
! 520             return np.array([self.source_time._freq0])
! 521         freq_min, freq_max = self.source_time.frequency_range_sigma(sigma=CHEB_GRID_WIDTH)
  522         if not self._is_fixed_angle:
  523             # For frequency-dependent angles (constat in-plane k), truncate minimum frequency at
  524             # the critical frequency of glancing incidence
! 525             f_crit = self.source_time._freq0 * np.sin(self.angle_theta)
  526             freq_min = max(freq_min, f_crit * CRITICAL_FREQUENCY_FACTOR)
  527         return self._chebyshev_freq_grid(freq_min, freq_max)
  528 
  529     def _post_init_validators(self) -> None:

tidy3d/components/source/time.py

  91 
  92     @cached_property
  93     def _freq0(self) -> float:
  94         """Central frequency. If not present in input parameters, returns `_freq0_sigma_centroid`."""
! 95         return self._freq0_sigma_centroid
  96 
  97     @cached_property
  98     def _freq0_sigma_centroid(self) -> float:
  99         """Central of frequency range at 1-sigma drop from the peak amplitude."""

  228         end_time = self.offset_time + END_TIME_FACTOR_GAUSSIAN * self.twidth
  229 
  230         # for derivative Gaussian that contains two peaks, add time interval between them
  231         if self.remove_dc_component and self.fwidth > self.freq0:
! 232             end_time += 2 * self._peak_time_shift
  233         return end_time
  234 
  235     def amp_freq(self, freq: float) -> complex:
  236         """Complex-valued source spectrum in frequency domain."""

  237         phase = np.exp(1j * self.phase + 1j * 2 * np.pi * (freq - self.freq0) * self.offset_time)
  238         envelope = np.exp(-((freq - self.freq0) ** 2) / 2 / self.fwidth**2)
  239         amp = 1j * self.amplitude / self.fwidth * phase * envelope
  240         if not self.remove_dc_component:
! 241             return amp
  242 
  243         # derivative of Gaussian when DC is removed
  244         return freq * amp / (2 * np.pi * self.peak_frequency)

  250     @property
  251     def peak_frequency(self) -> float:
  252         """Frequency at which the source time dependence has its peak amplitude in the frequency domain."""
  253         if not self.remove_dc_component:
! 254             return self.freq0
  255         return 0.5 * (self.freq0 + np.sqrt(self.freq0**2 + 4 * self.fwidth**2))
  256 
  257     @property
  258     def _peak_freq_amp(self) -> complex:

  261 
  262     @property
  263     def _peak_time_amp(self) -> complex:
  264         """Peak amplitude in time domain"""
! 265         return self.amp_time(self.peak_time)
  266 
  267     def frequency_range_sigma(self, sigma: float = DEFAULT_SIGMA) -> FreqBound:
  268         """Frequency range where the source amplitude is within ``exp(-sigma**2/2)`` of the peak amplitude."""
  269         if not self.remove_dc_component:

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-3541-more-accurate-frequency-range branch from ad9d0fb to 2886bf1 Compare October 8, 2025 00:34
Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Great!

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-3541-more-accurate-frequency-range branch from 2886bf1 to 0e26a65 Compare October 8, 2025 16:05
@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-3541-more-accurate-frequency-range branch from 0e26a65 to 25f2f30 Compare October 8, 2025 17:46
@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-3541-more-accurate-frequency-range branch from 25f2f30 to fd34d8d Compare October 8, 2025 22:57
@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Oct 8, 2025
Merged via the queue into develop with commit 8b44dc5 Oct 9, 2025
25 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/FXC-3541-more-accurate-frequency-range branch October 9, 2025 00:05
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