feat: downsample frequencies in mode monitors (FXC-3351) #2923
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/data/dataset.pytidy3d/components/data/monitor_data.pytidy3d/components/microwave/data/monitor_data.pytidy3d/components/mode/mode_solver.pytidy3d/components/mode_spec.py |
dbochkov-flexcompute
changed the title
dbochkov-flexcompute
force-pushed
the
daniil/FXC-3351-Downsample-frequencies-in-mode-monitors
branch
2 times, most recently
from
c070488 to
19df1ca
Compare
dbochkov-flexcompute
requested review from
caseyflex,
momchil-flex and
weiliangjin2021
momchil-flex
left a comment
momchil-flex
left a comment
There was a problem hiding this comment.
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?
| "ModeAmpsDataArray", | ||
| "ModeData", | ||
| "ModeIndexDataArray", | ||
| "ModeInterpSpec", |
There was a problem hiding this comment.
need to also add it in "all" later on in the file
| freqs, | ||
| "nearest", | ||
| ).data | ||
| ) |
There was a problem hiding this comment.
In my sort_spec modification I introduced a better way to iterate all relevant arrays without the risk of forgetting something (or forgetting to add it if added in the future):
tidy3d/tidy3d/components/data/monitor_data.py
Lines 2222 to 2233 in 19df1ca
There was a problem hiding this comment.
btw, regarding grid correction factors, do you think it's ok to interpolate or better recalculate after interpolation?
| Monitor that can use this specification to reduce mode computation cost. | ||
| :class:`ModeMonitor`: | ||
| Monitor that can use this specification to reduce mode computation cost. |
There was a problem hiding this comment.
My first thought was that the interp spec belongs in ModeSpec.
However, reading through I think I see why you took this route. We might want to set different default values in different objects, which could get quite awkward if we only have a single shared place to set the default value. Furthermore, ModeSource-s should have this off (either error or ignore it if set) as they already have their num_freqs for Chebyshev interpolation.
On the other hand, I'm not a huge fan of having it outside of ModeSpec either. I think because of that, you've missed adding the parameter in at least two places: ModalPort (commented below) and ModeSimulation.
All things considered though it might be best to be separate (just be careful about adding it everywhere...)
There was a problem hiding this comment.
I would also prefer this in ModeSpec. We can still set the defaults separately in different places, like WavePort.mode_spec defaults to ModeSpec(interp_spec=DEFAULT_WAVE_PORT_INTERP_SPEC)
There was a problem hiding this comment.
The problem I'm anticipating is when you want to set a non-default one. Say default ModeSpec doesn't interpolate but default ModeMonitor.mode_spec does:
ms = td.ModeSpec(angle_phi=np.pi / 4)
monitor = td.ModeMonitor(mode_spec=ms) # now this won't interpolate anymore
There was a problem hiding this comment.
oh I see. Well, we already have ModeSpecType = Union[ModeSpec, MicrowaveModeSpec]. Maybe we should introduce more subclasses like WavePortModeSpec, if we want different default mode specification behavior? I still think everything related to the mode solver settings should be in the shared class ModeSpec
There was a problem hiding this comment.
we actually have EMEModeSpec too already
There was a problem hiding this comment.
yeah, no strong preference here from me, just unsure about having this information in mode_spec when some objects, ModeSource and ModeABCBoundary, will not use it. Another option to make it a bit cleaner while not putting ModeInterpSpec into ModeSpec is to create a mixin ModeInterpolationModel and make all revelant classes (ModeMonitor, ModeSolverMonitor, WavePort, ModalPort, ModeSimulation) to inherit from it
There was a problem hiding this comment.
oh I see. Well, we already have ModeSpecType = Union[ModeSpec, MicrowaveModeSpec]. Maybe we should introduce more subclasses like WavePortModeSpec, if we want different default mode specification behavior? I still think everything related to the mode solver settings should be in the shared class ModeSpec
Just trying to go with this to see how it feels:
- Say that we ignore interpolation spec in ModeSource (this in itself is a bit confusing but potentially OK). Then I can still use regular ModeSpec in ModeSource, ModeMonitor, ModeSolverMonitor, ModeSolver, ModeSimulation, ModalPort.
- For all microwave objects like MicrowaveModeMonitor, WavePort, we use MicrowaveModeSpec. By the way @dmarek-flex I notice that WavePort expects a ModeSpec rather than a MicrowaveModeSpec, is that a mistake?
we actually have EMEModeSpec too already
- In EME, we've been talking about a different setting to only simulate at the central frequency. But I guess separately from that it could benefit from this frequency interpolation as an approach that would be more accurate than central frequency only but not as computationally heavy as all freqs. So potentially the interpolation is still used in EMEModeSpec, with even different defaults if needed.
I guess overall it doesn't sound too bad, especially if the user is already forced to use MicrowaveModeSpec or EMEModeSpec in the corresponding objects, which I think should be the case?
Then for Daniil's suggestion:
Another option to make it a bit cleaner while not putting ModeInterpSpec into ModeSpec is to create a mixin ModeInterpolationModel and make all revelant classes (ModeMonitor, ModeSolverMonitor, WavePort, ModalPort, ModeSimulation) to inherit from it
Not opposed to that either, but apart from inheriting the field, there could be various places where we have to be careful that it's correctly passed. At least when it's in a ModeSpec, it should be passed around by default in most places.
There was a problem hiding this comment.
- By the way @dmarek-flex I notice that WavePort expects a ModeSpec rather than a MicrowaveModeSpec, is that a mistake?
It is part of these bigger changes:
#2846
tidy3d/plugins/smatrix/ports/wave.py
Outdated
| description="Extrudes structures that intersect the wave port plane by a few grid cells when ``True``, improving mode injection accuracy.", | ||
| ) | ||
|
|
||
| interp_spec: Optional[ModeInterpSpec] = pd.Field( |
There was a problem hiding this comment.
We want to have this as an option in ModalPort-s too even if the default is still None.
caseyflex
left a comment
caseyflex
left a comment
There was a problem hiding this comment.
This is awesome @dbochkov-flexcompute !
| return self._get_data_with_group_index() | ||
|
|
||
| if self.interp_spec is not None and self.interp_spec.num_points < len(self.freqs): | ||
| return self._get_data_with_interp() |
There was a problem hiding this comment.
I'm wondering about the interaction with group_index_step. Maybe if both are set, we should use the interpolated data to obtain the group index?
There was a problem hiding this comment.
I need to improve on it indeed. What currently happens, if group_index_step is not False and interp_spec is not None:
_get_data_with_group_indexintercepts original mode solver and creates a new mode solver with additional frequency points_get_data_with_interpintercepts this modified mode solver and interpolates modes to all frequency points (including the additional aones)_get_data_with_group_indexuses interpolated values to calculate group index and dispersion
I'm thinking of modifying the behavior in the following way:
group_index_stepis notFalseandinterp_specislinear: error, because linear interpolation is not enough for computing second order derivatives for dispersion- if
group_index_step=Trueandinterp_specis notNone: obtain group index and dispersion by differentiating underlying interpolators directly, without first interpolating to additional points - if
group_index_stepis a specific number andinterp_specis notNone: obtain group index and dispersion as it's currently done, that is, by using interpolated values
Maybe @lucas-flexcompute would also have some opinion on that?
There was a problem hiding this comment.
Actually I don't really see the benefit of your proposed approach. In the current approach, the group index will be computed relatively correctly at the sampling frequencies, or at least equally correctly to the way we're computing it now. Then we just interpolate that. The group index is usually smooth. The GVD might have stronger dependence but even that shouldn't be too bad.
In your proposal, this would be the default way people do the computation.
if group_index_step=True and interp_spec is not None: obtain group index and dispersion by differentiating underlying interpolators directly, without first interpolating to additional points
Are you suggesting it for higher accuracy or for increased speed compared to the current approach? I think we could consider it, but only if the accuracy of the GVD is higher.
There was a problem hiding this comment.
Sorry, I just reread your description of what the current setup does, and I see the issue. The reason I posted the comment above was because in my mind something else was happening. This is what I thought:
- Interpolation frequencies at which modes will be computed are selected
- If group index is enabled, the additional frequencies around these are added
- The group index and GVD are computed with the same accuracy as it is now at those frequencies
- They are then interpolated to the other solver frequencies just as n_complex, etc.
Does that make sense as the workflow to implement?
There was a problem hiding this comment.
yeah, I see, I think this is the matter of simply swapping the order of _get_data_with_group_index and _get_data_with_interp. The only concerns is that we are tripling number of mode solves, so being able to, say, use the underlying chebyshev interpolator could be nice as an advanced option
There was a problem hiding this comment.
Maybe @lucas-flexcompute would also have some opinion on that?
Not strongly… My understanding is that the interpolation error should probably already be less than the error we get from the effective index due to discretization already. If interpolating over more points and getting the derivatives from the interpolation helps, solving for 3× the number of frequencies is not a big deal from the user perspective.
There was a problem hiding this comment.
@dbochkov-flexcompute I think we should go with the approach of getting the "equivalent accuracy" group index at the sampling points (even if it triples the number of solves, as it has until now) just because it matches legacy usage and just overall keeps the usage straightforward. I'm not a huge fan for example of not allowing group_index_step=True with interpolation=linear. Maybe just in the future we could experiment with using the interpolators to compute the derivatives if group_index_step=True and interpolation is not linear.
| Monitor that can use this specification to reduce mode computation cost. | ||
| """ | ||
|
|
||
| num_points: int = pd.Field( |
There was a problem hiding this comment.
I have a use case which is not quite covered by the current API. I want to know the mode profiles at a single frequency, say the central frequency; but I don't care about how they change wrt frequency. I do care about how the dispersion relation changes wrt frequency, and ideally I would compute that with interpolation.
Currently, I would need to do two separate mode solves; one at the central frequency recording the fields, and one over the full frequency range just storing the n_complex. This is a bit inefficient and perhaps cumbersome.
I wonder if we can add a flag to mode_spec like store_fields_at_all_freqs. If False, we just store the fields at the central frequency, while storing the index at all frequencies? This introduces some misalignment between the field and n_complex data arrays, but it can be handled reasonably by broadcasting where needed?
Also I think it would have been better if fields and conjugated_dot_product were part of mode_spec too.
@momchil-flex what do you think?
There was a problem hiding this comment.
or assume_constant_fields. Basically that could be the API for interpolating the n_complex but only storing one copy of the mode fields
There was a problem hiding this comment.
I think it's a useful suggestion. As another example I think in photonforge @lucas-flexcompute has to get the fields, but can probably do all that's needed using the central frequency fields only. This could drastically reduce the amount of data that's downloaded. I mean in principle there's a workaround to this already: you could place a ModeSolverMonitor with a single frequency at the same position as the ModeMonitor. That was kind of the original purpose of the ModeSolverMonitor but we eventually kind of unified the two.
It would be nice to have a more native support yeah, although it definitely increases the complexity. So in any case I think that's something that should be done in a separet PR.
Also I think it would have been better if fields and conjugated_dot_product were part of mode_spec too
I guess this, as the current interp spec, has to do with the fact that they don't take effect in a ModeSource. Actually, conjugated_dot_product is only really used in a ModeMonitor. It seems that it is an attribute to the ModeSolver just so that it can be passed in to_monitor. But I feel like it should instead be passed as an argument to that method, i.e. add a kwargs as in to_source? Looping @dmarek-flex here too. It's not yet released officially so we could remove it from the ModeSolver.
There was a problem hiding this comment.
Actually,
conjugated_dot_productis only really used in aModeMonitor. It seems that it is an attribute to theModeSolverjust so that it can be passed into_monitor. But I feel like it should instead be passed as an argument to that method, i.e. add akwargsas into_source? Looping @dmarek-flex here too. It's not yet released officially so we could remove it from the ModeSolver.
I might have jumped the gun adding conjugated_dot_product to the ModeSolver and ModeSimulation, but I will need to add some Microwave-specific features for post-processing modes that I think is best done in the backend mode solver, so I do think it might be convenient to have that field around in the future. On the other hand, maybe internally I don't need to consider the conjugated version for my uses.
There was a problem hiding this comment.
I think it's a useful suggestion. As another example I think in photonforge @lucas-flexcompute has to get the fields, but can probably do all that's needed using the central frequency fields only.
PF needs the fields at a single frequency indeed only for computing mode number remapping (if the user uses symmetry or filter_pol) so that we can connect to the correct modes in other parts of the circuit where those might not have been used. But we build this as a projection operation (not index search) because there's no guarantee that the "same" modes will be found with and without symmetry due to degenerate mode mixing.
In the end, I think we might just keep using the full frequency data (at least in the short term) because it is easy to implement.
weiliangjin2021
left a comment
weiliangjin2021
left a comment
There was a problem hiding this comment.
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?
| if self.method in ("linear", "cubic"): | ||
| # Uniformly spaced points | ||
| return np.linspace(f_min, f_max, self.num_points) | ||
| elif self.method == "cheb": |
There was a problem hiding this comment.
pylint would complain no else is needed
|
|
||
| # Check relative error | ||
| freq_range = np.abs(expected_freqs[-1] - expected_freqs[0]) | ||
| max_error = np.max(np.abs(freqs_sorted - expected_sorted)) / freq_range |
There was a problem hiding this comment.
will np.allclose with custom atol and rtol be more robust here?
| # Interpolate field components if present | ||
| for field_name, field_data in self.field_components.items(): | ||
| if field_data is not None: | ||
| update_dict[field_name] = self._interp_dataarray(field_data, freqs, method) |
There was a problem hiding this comment.
This runs the risk of being very slow, did you benchmark it at all?
| mode_solver_reduced = self.copy(update={"freqs": freqs_reduced, "interp_spec": None}) | ||
|
|
||
| # Get data at reduced frequencies | ||
| data_reduced = mode_solver_reduced.data_raw |
There was a problem hiding this comment.
Can you use _data_on_yee_grid here instead, and then do the processing later in data_raw?
There was a problem hiding this comment.
@dbochkov-flexcompute I still can't use this directly in EME unless you use _data_on_yee_grid instead of data_raw here. It might take reworking the group index handling a bit too, but I think it is worth it
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
lucas-flexcompute
left a comment
There was a problem hiding this comment.
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 |
dbochkov-flexcompute
force-pushed
the
daniil/FXC-3351-Downsample-frequencies-in-mode-monitors
branch
2 times, most recently
from
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 |
dbochkov-flexcompute
force-pushed
the
daniil/FXC-3351-Downsample-frequencies-in-mode-monitors
branch
2 times, most recently
from
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 |
|
|
||
| return mode_solver.data_raw._group_index_post_process(self.mode_spec.group_index_step) | ||
|
|
||
| def _get_data_with_interp(self) -> ModeSolverData: |
There was a problem hiding this comment.
@dbochkov-flexcompute Maybe it would be better to directly incorporate this method into _data_on_yee_grid. That is, define a property ModeSolver.sampling_points which is either freqs or sampling_points(freqs); then _data_on_yee_grid always computes at sampling_points. Then interpolated_data can be called, depending on reduce_data, either there or in data_raw.
There was a problem hiding this comment.
Yeah, I think this should be doable now with .interpolated_copy property now, because we do need to wait until mode tracking is done and until group_index/dispersion are calculated to do interpolation
tidy3d/components/base.py
Outdated
| return _LazyProxy | ||
|
|
||
|
|
||
| class InterpolatableMixin(Tidy3dBaseModel): |
There was a problem hiding this comment.
I don't understand why this has to be a mixin, instead of an extra field in ModeInterpSpec
There was a problem hiding this comment.
also you may need a unit test for reduce_data, not sure it works currently
There was a problem hiding this comment.
Since ModeSpec/ModeInterpSpec are used in so many places I think it might get a bit confusing for the user to remember what options in those specs apply to which objects. With including interp_spec into ModeSpec it is not so bad as distinction there is simple: it applies only to monitors. But distinction for reduce_data is more fine grained: it applies to ModeSolverMonitor and ModeSolver (ModeSimulation and EMEModeSolverMonitor in future?), but not ModeMonitor. It also avoids accidentally turning on this feature where we do not want/anticipate it.
There was a problem hiding this comment.
Can we not just have different defaults / validation in these different places?
It's not the end of the world, but for example in EME, it is convenient to have all of the arguments for the mode solver kept in ModeSpec -- then we can simply have one mode spec per EME cell, rather than tracking additional data for every cell too. It is cleaner from my perspective to have everything in one place
| ) | ||
| return val | ||
|
|
||
| _warn_interp_num_points = validate_interp_num_points() |
There was a problem hiding this comment.
Can we put this warning in data_raw instead?
caseyflex
left a comment
caseyflex
left a comment
There was a problem hiding this comment.
@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
dbochkov-flexcompute
force-pushed
the
daniil/FXC-3351-Downsample-frequencies-in-mode-monitors
branch
from
0670c55 to
0d64dad
Compare
Addresses #2530.
Notebooks re-run examples:
Still not sure about making it default in WavePorts
Greptile Overview
Updated On: 2025-10-23 01:43:36 UTC
Greptile Summary
This PR implements frequency downsampling/interpolation for mode monitors and mode solvers to reduce computational cost in broadband simulations. A new
ModeInterpSpecclass is introduced with three interpolation methods (linear, cubic, Chebyshev), allowing modes to be computed at fewer strategically-chosen frequencies and interpolated to the full frequency set. The feature is integrated throughout the mode computation pipeline: fromWavePortthroughModeMonitorandModeSolvertoModeSolverData. The implementation requires mode tracking to be enabled (mode_spec.sort_spec.track_freq) to maintain consistent mode ordering across frequencies. Default behavior changes for wave ports now use 21-point Chebyshev interpolation unless explicitly disabled. The implementation includes comprehensive validation, extrapolation warnings, and extensive test coverage.Important Files Changed
Changed Files
ModeInterpSpecclass in public API with proper alphabetical orderingModeInterpSpecclass with three interpolation methods and validation logicinterp_specfield toAbstractModeMonitorwith tracking requirement validationModeSolverbut contains debug print statements requiring removalinterp()method toModeSolverDatasupporting all three interpolation methodsConfidence score: 3/5
num_pointsfrom exceeding total frequencies inModeInterpSpec, and (4) lack of explicit handling for single-frequency edge case insampling_points()method.tidy3d/components/mode/mode_solver.py(remove debug prints),tidy3d/plugins/smatrix/ports/wave.py(default behavior change may need migration guidance), andtidy3d/components/mode_spec.py(add upper bound validation fornum_points).Sequence Diagram
sequenceDiagram participant User participant ModeSolver participant ModeInterpSpec participant ModeSolverData participant compute_modes User->>ModeSolver: Create ModeSolver with interp_spec ModeSolver->>ModeInterpSpec: Validate num_points >= method requirements ModeInterpSpec-->>ModeSolver: Validation complete User->>ModeSolver: Call data_raw property ModeSolver->>ModeSolver: Check if interp_spec is provided alt interp_spec is None ModeSolver->>ModeSolver: Call _data_on_yee_grid() ModeSolver->>compute_modes: Solve at all freqs compute_modes-->>ModeSolver: Return n_complex, fields, eps_spec ModeSolver-->>User: Return ModeSolverData else interp_spec provided ModeSolver->>ModeSolver: Call _get_data_with_interp() ModeSolver->>ModeInterpSpec: sampling_points(freqs) alt method is "linear" or "cubic" ModeInterpSpec->>ModeInterpSpec: Generate uniformly spaced points else method is "cheb" ModeInterpSpec->>ModeInterpSpec: Generate Chebyshev nodes end ModeInterpSpec-->>ModeSolver: Return freqs_reduced ModeSolver->>ModeSolver: Create mode_solver_reduced with freqs_reduced ModeSolver->>ModeSolver: Call data_raw on mode_solver_reduced ModeSolver->>compute_modes: Solve at reduced freqs compute_modes-->>ModeSolver: Return data_reduced ModeSolver->>ModeSolverData: interp(freqs, method) ModeSolverData->>ModeSolverData: _interp_dataarray for each field alt method is "cheb" ModeSolverData->>ModeSolverData: Validate Chebyshev nodes ModeSolverData->>ModeSolverData: Use barycentric interpolation else method is "linear" or "cubic" ModeSolverData->>ModeSolverData: Use xarray interpolation end ModeSolverData-->>ModeSolver: Return interpolated data ModeSolver-->>User: Return ModeSolverData with all freqs endContext used:
dashboard- Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)