-
Notifications
You must be signed in to change notification settings - Fork 65
feat: downsample frequencies in mode monitors (FXC-3351) #2923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: downsample frequencies in mode monitors (FXC-3351) #2923
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, 9 comments
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/data/dataset.pyLines 169-177 169 num_freqs, num_modes = sort_inds_2d.shape
170 modify_data = {}
171 for key, data in self.data_arrs.items():
172 if "mode_index" not in data.dims or "f" not in data.dims:
! 173 continue
174 dims_orig = data.dims
175 f_coord = data.coords["f"]
176 slices = []
177 for ifreq in range(num_freqs):tidy3d/components/mode/mode_solver.pyLines 477-485 477 return self.data
478
479 def _freqs_for_group_index(self, freqs: FreqArray) -> FreqArray:
480 """Get frequencies used to compute group index."""
! 481 return self.mode_spec._freqs_for_group_index(freqs=self.freqs)
482
483 @cached_property
484 def _sampling_freqs(self) -> FreqArray:
485 """Get frequencies used to compute group index and interpolation."""Lines 618-629 618 # TODO: At a later time, we should ensure that `eps_spec` is automatically returned to
619 # to compute the backward propagation mode solution using a mode solver
620 # with direction "-".
621 eps_spec = []
! 622 for _ in solver.freqs:
623 eps_spec.append("tensorial_complex")
624 # finite grid corrections
! 625 grid_factors, relative_grid_distances = solver._grid_correction(
626 simulation=solver.simulation,
627 plane=solver.plane,
628 mode_spec=solver.mode_spec,
629 n_complex=n_complex,Lines 985-993 985 # Extract coordinate values from one of the six field components
986 xyz_coords = solver.grid_snapped[field_name].to_list
987 x, y, z = (coord.copy() for coord in xyz_coords)
988
! 989 f = np.atleast_1d(self._sampling_freqs)
990 mode_index = np.arange(self.mode_spec.num_modes)
991
992 # Initialize output arrays
993 shape = (x.size, y.size, z.size, len(f), mode_index.size)Lines 1975-1983 1975 """Check if there are media with a complex-valued epsilon in the plane of the mode.
1976 A separate check is done inside the solver, which looks at the actual
1977 eps and mu and uses a tolerance to determine whether to use real or complex fields, so
1978 the actual behavior may differ from what's predicted by this property."""
! 1979 check_freqs = np.unique(
1980 [
1981 np.amin(self._sampling_freqs),
1982 np.amax(self._sampling_freqs),
1983 np.mean(self._sampling_freqs),tidy3d/components/mode_spec.pyLines 197-205 197 def _validate_freqs(cls, val):
198 """Validate custom frequencies."""
199 freqs_array = np.asarray(val)
200 if freqs_array.size < 2:
! 201 raise ValidationError("Custom sampling requires at least 2 frequency points.")
202 return val
203
204 def sampling_points(self, freqs: FreqArray) -> FreqArray:
205 """Return the custom frequency sampling points.Lines 294-302 294 def _validate_method_needs_points(cls, val, values):
295 """Validate that the method has enough points."""
296 sampling_spec = values.get("sampling_spec")
297 if sampling_spec is None:
! 298 return val
299
300 num_points = sampling_spec._num_points
301 if val == "cubic" and num_points < 4:
302 raise ValidationError(Lines 681-689 681 if track_freq is not None:
682 return track_freq
683 if sort_spec is not None:
684 return sort_spec.track_freq
! 685 return None
686
687 @pd.validator("interp_spec", always=True)
688 @skip_if_fields_missing(["sort_spec", "track_freq"])
689 def _interp_spec_needs_tracking(cls, val, values):tidy3d/components/monitor.pyLines 822-830 822 """Size of monitor storage given the number of points after discretization."""
823 amps_size = 3 * BYTES_COMPLEX * len(self._stored_freqs) * self.mode_spec.num_modes
824 fields_size = 0
825 if self.store_fields_direction is not None:
! 826 fields_size = (
827 6 * BYTES_COMPLEX * num_cells * len(self._stored_freqs) * self.mode_spec.num_modes
828 )
829 if self.mode_spec.precision == "double":
830 fields_size *= 2 |
c070488 to
19df1ca
Compare
momchil-flex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I didn't know this was coming!
I think we might want to enable it by default for ModeMonitor-s, or at least for all ports. But we could also sit on it for a bit, maybe let @lucas-flexcompute test this out as a default in photonforge first?
caseyflex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome @dbochkov-flexcompute !
weiliangjin2021
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very pleasant PR to review: both feature-rich, but not so many lines of code change!
As a future step, maybe it's useful to support adaptive sampling, as the mode profile probably changes more rapidly in high-frequency compared to low-frequency range. Or perhaps expose an API for custom sampling frequencies for now?
Yes I was thinking about suggesting the same thing. Another application of custom sampling frequencies could be when you have a mode crossing, you could put more frequencies around it so that the tracking works. |
lucas-flexcompute
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if this is a dumb question, but I need to understand it first: is the ModeInterpSpec a superset of what ModeSource.num_freqs already does? How do they combine?
There is definitely a big overlap between them, but @momchil-flex @caseyflex this could actually make the decision whether to put |
So are you driving towards separating it also because of this argument? And I guess deprecating
I don't think so as the field computation is just analytical. Maybe a tiny benefit to interpolating. |
|
@dbochkov-flexcompute I think it makes sense to incorporate it into |
|
I think I will move |
eb1a1f8 to
18f3fcd
Compare
|
Did a revision of this (turned out a bit more than expected):
Still not sure whether we need to worry much about interpolating grid correction factor or better to recompute them. Another question is whether we want to implement any sort of automatic fall back strategy "cheb" -> "cubic" -> "linear" -> "nearest" of we detect a mode discontinuity |
|
I think it's better to recompute the grid correction factor, interpolating the complex exponential can lead to bad behavior (abs < 1) I'm not sure about a fallback, but I am going to add |
|
One thing I remembered, you should add the |
d58af9b to
07415ba
Compare
Agree. I did have to add some new fields (
|
Curious about this - the grid should be the same at all frequencies, no? |
yeah, just after n_complex is interpolated onto new set of frequencies, we need to recompute |
caseyflex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbochkov-flexcompute I left a few more comments, but I'd like to get this in soon, so I can clean up my EME PR. I guess the important thing is to get the API stabilized -- maybe we can do that and merge, and then I can change the internals while working on EME. To that end, I'd just consider moving reduce_data into ModeInterpSpec as the primary API change, then see if we can get this merged
0670c55 to
0d64dad
Compare
bdd7b09 to
aedc3a9
Compare
caseyflex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, very flexible and extensible. thanks @dbochkov-flexcompute !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
18 files reviewed, no comments
2daa477 to
3571d01
Compare
3571d01 to
73e8ac0
Compare
Addresses #2530.
Notebooks re-run examples:
Still not sure about making it default in WavePorts
Greptile Overview
Greptile Summary
This PR implements frequency downsampling/interpolation for mode monitors and mode solvers to reduce computational cost in broadband simulations. The implementation introduces a new
ModeInterpSpecclass with three interpolation methods (linear, cubic, Chebyshev polynomial), allowing modes to be computed at fewer strategically-chosen frequencies and interpolated to the full frequency set.Key Changes:
ModeInterpSpecclass withUniformSampling,ChebSampling, andCustomSamplingstrategies for flexible frequency selectionModeSpec,AbstractModeMonitor,ModeSolver, andModeSolverDataModeSolverData.interp()supporting all three methods with proper handling of complex-valued datamode_spec.sort_spec.track_freq) to maintain consistent mode ordering across frequenciesNotable Implementation Details:
reduce_dataflag allows storing only sampled frequencies to minimize storage costsConfidence Score: 4/5
num_pointsinModeInterpSpecthat could exceed total frequencies, (2) f-string usage in warning message violating style guide rule 9b45957a-2d0d-4c24-a5d3-eef6f721cbfd, (3) potential edge case with single-frequency inputs not explicitly handled, and (4) extrapolation tolerance using multiplicative factor may behave unexpectedly near zero frequenciestidy3d/components/mode_spec.py(validation gaps) andtidy3d/components/monitor.py(style guide violation)Important Files Changed
File Analysis
ModeInterpSpecwith three interpolation methods and sampling strategies; validation ensuresnum_pointsmeets method requirements but lacks upper bound check and single-frequency edge case handlinginterp_specfield toAbstractModeMonitorwith tracking requirement validation; f-string usage in warning violates style guideModeSolver; debug print statements have been removed (no longer present in latest commit)interp()method toModeSolverDatasupporting all interpolation methods; extrapolation tolerance uses multiplicative factor which may be problematic near zero frequenciesSequence Diagram
sequenceDiagram participant User participant ModeSolver participant ModeInterpSpec participant ModeSolverData participant compute_modes User->>ModeSolver: Create with interp_spec ModeSolver->>ModeInterpSpec: Validate num_points ModeInterpSpec-->>ModeSolver: Validation complete User->>ModeSolver: Call data_raw property alt interp_spec is None ModeSolver->>compute_modes: Solve at all frequencies compute_modes-->>ModeSolver: Return raw data ModeSolver-->>User: ModeSolverData (all freqs) else interp_spec provided ModeSolver->>ModeInterpSpec: sampling_points(freqs) alt method is linear/cubic ModeInterpSpec->>ModeInterpSpec: Generate uniform points else method is cheb ModeInterpSpec->>ModeInterpSpec: Generate Chebyshev nodes end ModeInterpSpec-->>ModeSolver: Return freqs_reduced ModeSolver->>ModeSolver: Create mode_solver_reduced ModeSolver->>compute_modes: Solve at reduced freqs compute_modes-->>ModeSolver: Return data_reduced ModeSolver->>ModeSolverData: interp(freqs, method) alt method is cheb ModeSolverData->>ModeSolverData: Barycentric interpolation else method is linear/cubic ModeSolverData->>ModeSolverData: xarray interpolation end ModeSolverData-->>ModeSolver: Interpolated data ModeSolver-->>User: ModeSolverData (all freqs) end