Skip to content

Conversation

@momchil-flex
Copy link
Collaborator

@momchil-flex momchil-flex commented Oct 16, 2025

Greptile Overview

Updated On: 2025-10-16 09:46:16 UTC

Greptile Summary

This PR adds Gaussian overlap monitor functionality to the Tidy3D simulation system through FXC-3275. The implementation introduces two new monitor classes: GaussianOverlapMonitor for circular Gaussian beam overlap calculations and AstigmaticGaussianOverlapMonitor for elliptical beams with different waist parameters in each direction.

The change refactors the monitor class hierarchy by creating a new AbstractOverlapMonitor base class that contains shared functionality between existing mode monitors and the new Gaussian monitors. This includes fields for store_fields_direction, colocate, and conjugated_dot_product, which were moved from AbstractModeMonitor to enable code reuse. The Gaussian monitors add specialized fields for beam parameters including angles, polarization, and waist characteristics.

The implementation follows established patterns in the codebase with proper inheritance, documentation, and type safety. Test coverage is included for both monitor classes with basic functionality verification and integration into the comprehensive monitor test suite.

Important Files Changed

Changed Files
Filename Score Overview
tidy3d/components/monitor.py 4/5 Major refactoring adding AbstractOverlapMonitor base class and two new Gaussian overlap monitor implementations
tidy3d/components/types/monitor.py 5/5 Simple type additions registering the new monitor classes in the type system
tests/test_components/test_monitor.py 5/5 Basic test coverage for the new monitor classes with integration into existing test suite

Confidence score: 4/5

  • This PR is safe to merge with good architectural design and proper testing
  • Score reflects solid implementation following established patterns, but monitor hierarchy refactoring in critical simulation code requires careful validation
  • Pay close attention to the monitor.py file to ensure the refactoring doesn't break existing mode monitor functionality

Sequence Diagram

sequenceDiagram
    participant User
    participant Simulation as "Simulation"
    participant GaussianMonitor as "GaussianOverlapMonitor"
    participant AstigmaticGaussianMonitor as "AstigmaticGaussianOverlapMonitor"
    participant AbstractGaussianOverlapMonitor as "AbstractGaussianOverlapMonitor"
    participant FreqMonitor as "FreqMonitor"
    participant Solver as "FDTD Solver"

    User->>GaussianMonitor: "Create GaussianOverlapMonitor(size, freqs, waist_radius, waist_distance)"
    GaussianMonitor->>AbstractGaussianOverlapMonitor: "Inherit base functionality"
    AbstractGaussianOverlapMonitor->>FreqMonitor: "Inherit frequency domain processing"
    
    User->>AstigmaticGaussianMonitor: "Create AstigmaticGaussianOverlapMonitor(size, freqs, waist_sizes, waist_distances)"
    AstigmaticGaussianMonitor->>AbstractGaussianOverlapMonitor: "Inherit base functionality"
    
    User->>Simulation: "Add monitors to simulation"
    Simulation->>GaussianMonitor: "Validate monitor configuration"
    Simulation->>AstigmaticGaussianMonitor: "Validate monitor configuration"
    
    User->>Simulation: "Run simulation"
    Simulation->>Solver: "Initialize FDTD solver"
    Solver->>GaussianMonitor: "Record field data at frequencies"
    Solver->>AstigmaticGaussianMonitor: "Record field data at frequencies"
    
    GaussianMonitor->>AbstractGaussianOverlapMonitor: "Compute Gaussian beam overlap"
    AstigmaticGaussianMonitor->>AbstractGaussianOverlapMonitor: "Compute astigmatic Gaussian beam overlap"
    
    AbstractGaussianOverlapMonitor->>Solver: "Return complex amplitudes for +/- directions"
    Solver->>Simulation: "Store monitor data"
    Simulation->>User: "Return simulation results with Gaussian overlap data"
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.

3 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@momchil-flex momchil-flex force-pushed the momchil/gaussian_monitor branch from 61d5146 to 61cebca Compare October 16, 2025 11:34
@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2025

Diff Coverage

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

  • tidy3d/components/data/monitor_data.py (88.9%): Missing lines 1611,1634,1649
  • tidy3d/components/monitor.py (76.7%): Missing lines 388-392,397,401-402,470,549
  • tidy3d/plugins/smatrix/init.py (100%)
  • tidy3d/plugins/smatrix/analysis/modal.py (100%)
  • tidy3d/plugins/smatrix/component_modelers/modal.py (100%)
  • tidy3d/plugins/smatrix/ports/modal.py (100%)

Summary

  • Total: 127 lines
  • Missing: 13 lines
  • Coverage: 89%

tidy3d/components/data/monitor_data.py

Lines 1607-1615

  1607 
  1608     def normalize(self, source_spectrum_fn) -> AbstractOverlapData:
  1609         """Return copy of self after normalization is applied using source spectrum function."""
  1610         if self.amps is None:
! 1611             return self.copy()
  1612         source_freq_amps = source_spectrum_fn(self.amps.f)[None, :, None]
  1613         new_amps = (self.amps / source_freq_amps).astype(self.amps.dtype)
  1614         return self.copy(update={"amps": new_amps})

Lines 1630-1638

  1630         mnt = self.monitor
  1631         new_dir = "+" if mnt.store_fields_direction == "-" else "-"
  1632         update_dict = {"store_fields_direction": new_dir}
  1633         if hasattr(mnt, "direction"):
! 1634             update_dict["direction"] = new_dir
  1635         new_data["monitor"] = mnt.updated_copy(**update_dict)
  1636         return self.copy(update=new_data)
  1637 

Lines 1645-1653

  1645         self, dataset_names: list[str], fwidth: float
  1646     ) -> list[Union[CustomCurrentSource, PointDipole]]:
  1647         """Converts a :class:`.FieldData` to a list of adjoint current or point sources."""
  1648 
! 1649         raise NotImplementedError("Could not formulate adjoint source for overlap monitor output.")
  1650 
  1651 
  1652 class ModeData(ModeSolverDataset, AbstractOverlapData):
  1653     """

tidy3d/components/monitor.py

Lines 384-406

  384 
  385     @cached_property
  386     def _dir_arrow(self) -> tuple[float, float, float]:
  387         """Direction vector from angles used for overlap (children must define angles)."""
! 388         theta, phi = self._angles
! 389         dx = np.cos(phi) * np.sin(theta)
! 390         dy = np.sin(phi) * np.sin(theta)
! 391         dz = np.cos(theta)
! 392         return self.unpop_axis(dz, (dx, dy), axis=self.normal_axis)
  393 
  394     @property
  395     def _angles(self) -> tuple[float, float]:
  396         """Angle tuple (theta, phi) in radians. Children override to supply values."""
! 397         return (0.0, 0.0)
  398 
  399     def _storage_size_solver(self, num_cells: int, tmesh: ArrayFloat1D) -> int:
  400         """Default size of intermediate data; store all fields on plane for overlap."""
! 401         num_sample = len(getattr(self, "freqs", [0]))
! 402         return BYTES_COMPLEX * num_cells * num_sample * 6
  403 
  404 
  405 class AbstractModeMonitor(AbstractOverlapMonitor):
  406     """:class:`Monitor` that records mode-related data."""

Lines 466-474

  466         return ax
  467 
  468     @cached_property
  469     def _angles(self) -> tuple[float, float]:
! 470         return (self.mode_spec.angle_theta, self.mode_spec.angle_phi)
  471 
  472     @cached_property
  473     def _dir_arrow(self) -> tuple[float, float, float]:
  474         """Monitor direction normal vector in cartesian coordinates."""

Lines 545-553

  545     )
  546 
  547     @property
  548     def _angles(self) -> tuple[float, float]:
! 549         return (self.angle_theta, self.angle_phi)
  550 
  551     def storage_size(self, num_cells: int, tmesh: ArrayFloat1D) -> int:
  552         """Size of monitor storage given the number of points after discretization."""
  553         # store complex amplitudes for +/- directions

@momchil-flex momchil-flex force-pushed the momchil/gaussian_monitor branch from 61cebca to daddcfd Compare October 16, 2025 12:22
@momchil-flex momchil-flex force-pushed the momchil/gaussian_monitor branch from daddcfd to a61be33 Compare October 17, 2025 12:01
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.

2 participants