add impedance calculations to mode solver #2837
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dmarek-flex
force-pushed
the
dmarek/add_impedance_to_mode_data
branch
2 times, most recently
from
ceccb9e to
18ced37
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/geometry/utils.pyLines 567-575 567 else: # SnapType.Contract
568 min_upper_bound_idx += snap_margin
569 max_upper_bound_idx -= snap_margin
570 if max_upper_bound_idx < min_upper_bound_idx:
! 571 raise SetupError("The supplied 'snap_buffer' is too large for this contraction.")
572 min_snap = get_upper_bound(
573 interval_min, coords, min_upper_bound_idx, rel_tol=rtol, strict_bounds=strict_bounds
574 )
575 max_snap = get_lower_bound(tidy3d/components/microwave/base.pyLines 15-23 15 @pd.root_validator(pre=False)
16 def _warn_rf_license(cls, values):
17 # Skip warning during doctest runs
18 if not is_running_pytest():
! 19 log.warning(
20 "ℹ️ ⚠️ RF simulations are subject to new license requirements in the future. You have instantiated at least one RF-specific component.",
21 log_once=True,
22 )
23 return valuestidy3d/components/microwave/path_integrals/mode_plane_analyzer.pyLines 208-216 208 (min_b_3d, max_b_3d), mode_symmetry_3d, conductor_shapely
209 )
210
211 if len(conductor_shapely) < 1:
! 212 raise SetupError(
213 "No valid isolated conductors were found in the mode plane. Please ensure that a 'Structure' "
214 "with a medium of type 'PEC' or 'LossyMetalMedium' intersects the mode plane and is not touching "
215 "the boundaries of the mode plane."
216 )tidy3d/components/microwave/path_integrals/path_integral_factory.pyLines 123-132 123 v_integrals.append(None)
124 i_integrals.append(None)
125 continue
126 elif isinstance(impedance_spec, AutoImpedanceSpec):
! 127 v_spec = None
! 128 i_spec = auto_spec.current_spec
129 else:
130 v_spec = impedance_spec.voltage_spec
131 i_spec = impedance_spec.current_spectidy3d/components/mode/mode_solver.pyLines 1379-1387 1379 ) -> tuple[tuple[Optional[VoltageIntegralTypes]], tuple[Optional[CurrentIntegralTypes]]]:
1380 """Wrapper for making path integrals from the MicrowaveModeSpec. Note: overriden in the backend to support
1381 auto creation of path integrals."""
1382 if not self._has_microwave_mode_spec:
! 1383 raise ValueError(
1384 "Cannot make path integrals for when 'mode_spec' is not a 'MicrowaveModeSpec'."
1385 )
1386 return make_path_integrals(self.mode_spec)tidy3d/plugins/microwave/custom_path_integrals.pyLines 229-237 229
230 # Perform phase splitting into in and out of phase for each frequency separately
231 for term in path_currents:
232 if np.all(term.abs == 0):
! 233 continue
234
235 # Compare phase to reference for each frequency
236 phase_diff = term.angle - phase_reference
237 # Wrap phase difference to [-pi, pi] |
dmarek-flex
force-pushed
the
dmarek/add_impedance_to_mode_data
branch
from
6996084 to
290484a
Compare
tidy3d/components/mode_spec.py
Outdated
| f"but the number of modes requested is {num_modes}. Please either ensure that the " | ||
| "number of impedance specifications is equal to the number of modes or leave the " | ||
| "'MicrowaveModeSpec' field as 'None', if impedances are not needed." | ||
| ) |
There was a problem hiding this comment.
Is it common to have the same or different MicrowaveModeSpec for each mode?
There was a problem hiding this comment.
The intention here is to put any additional RF/Microwave specific settings which may control the overall results of the mode solver (like adding options for processing groups of degenerate modes). Right now it only has impedance_specs which is needed for each individual mode.
So the purpose of MicrowaveModeSpec is to mainly keep rf and photonics things a bit separate. Although we could add impedance_specs directly to ModeSpec, or MicrowaveModeSpec derive from ModeSpec. But then we might need like a MicrowaveModeMonitor. paging @daquinteroflex
There was a problem hiding this comment.
MicrowaveModeSpec derive (inherits) from ModeSpec
would be much better if you can do this @dmarek-flex ? I believe we have a goal to split things more cleanly also for all core fdtd and mode components
ie
class MicrowaveModeSpec(ModeSpec):
microwave_parameters1: ...
microwave_parameters2: ...
There was a problem hiding this comment.
I guess I should do the same with MicrowaveModeData?
There was a problem hiding this comment.
Yeah ideally, let me have a look across the PR iab then
There was a problem hiding this comment.
Ultimately, I had to revert back to a composition approach. Inheritance led to too many changes related to ModeData/ModeSolverData.
But I did manage to allow for broadcasting a single impedance spec to all modes
There was a problem hiding this comment.
Very impressive work, finished the first pass and left some comments. Additionally, it seems like many new classes AxisAlignedPathIntegralSpec, CustomPathIntegral2DSpec, CurrentIntegralAxisAlignedSpec, CustomCurrentIntegral2DSpec, CompositeCurrentIntegralSpec, CustomImpedanceSpec, etc are missing examples in docstrings
dmarek-flex
force-pushed
the
dmarek/add_impedance_to_mode_data
branch
from
7ad7a12 to
2727e2f
Compare
dmarek-flex
force-pushed
the
dmarek/add_impedance_to_mode_data
branch
from
917c3ad to
d23f936
Compare
|
Thanks @dbochkov-flexcompute for finding those docstring mistakes. It was a little bit harder than expected to get doc tests running correctly because of the log warning we have for rf and microwave components. I got it working by introducing the |
There was a problem hiding this comment.
Looks pretty good @dmarek-flex ! Just a few questions on very few API design considerations
| tidy3d.plugins.microwave.Custom2DCurrentIntegral | ||
| tidy3d.plugins.microwave.AxisAlignedPathIntegral | ||
| tidy3d.plugins.microwave.CustomPathIntegral2D | ||
| tidy3d.plugins.microwave.Custom2DPathIntegral |
There was a problem hiding this comment.
This was only introduced in rc1 right?
There was a problem hiding this comment.
Path integrals have been since ~2.7, but we are taking the opportunity here to get in some breaking changes before RF release.
There was a problem hiding this comment.
Hmm maybe we need a breaking changes section of 2.10
There was a problem hiding this comment.
We also need to update a bunch of notebooks too
There was a problem hiding this comment.
I have a notebook branch incorporating these changes.
Hmm maybe we need a breaking changes section of 2.10
Like in the docs or changelog?
There was a problem hiding this comment.
Maybe in Changelog given it's a special one
| "computed. Possible values are 'diagonal', 'tensorial_real', 'tensorial_complex'.", | ||
| ) | ||
|
|
||
| microwave_data: Optional[MicrowaveModeDataset] = pd.Field( |
There was a problem hiding this comment.
Sorry wasn't it meant to be a MicrowaveModeData instead of adding this parameter?
There was a problem hiding this comment.
See my other comment on ModeSpec
| GROUP_INDEX_STEP = 0.005 | ||
|
|
||
|
|
||
| class ModeSpec(Tidy3dBaseModel): |
There was a problem hiding this comment.
So yeah why simply not make a class MicrowaveModeSpec(ModeSpec) ?
There was a problem hiding this comment.
I did try this approach and it worked ok for the ModeSpec/MicrowaveModeSpec side of things. But then I felt like I would need to have a MicrowaveModeData/MicrowaveModeSolverData and that would require modifications all over the code base (frontend and backend) to provide the correct type hints and Pydantic fields.
And then following that logic we would need a MicrowaveModeMonitor/MicrowaveModeSolverMonitor/MicrowaveModeSimulation and it felt like maintaining the proper type hints would be a lot of extra work. From trying this out, inheritance did not seem the easiest approach for maintenance, and I decided that composition would be a better approach.
There was a problem hiding this comment.
@yaugenst-flex @tylerflex it'd be good to get your input here in terms of how we do separation of scope rather than mixing RF into the general ModeSpec as currently implemented in this PR per the following lines in this file. It is concievable that MicrowaveModeSpec(ModeSpec) keeps getting extended as well as future classes that Damian mentioned in order to provide custom RF functionality eventually, but it is a larger effort in terms of the API restructure that we also have to consider. The benefit though is a stronger separation of API, which is related to previous conversations we've had privately about product scope.
tidy3d/tidy3d/components/mode_spec.py
Lines 167 to 173 in a4c3c40
There was a problem hiding this comment.
Yea I'd rather make a dedicated MicrowaveModeSpec(ModeSpec) in a separate place vs mixing it into the component as microwave_spec: Optional[MicrowaveModeSpec] = pd.Field
eventually I think all of the RF-specific components should be moved into their own separate namespace, like a tidy3d.rf, and that includes terminal component modeler, MicrowaveModeSpec, and probably a separate RFSimulation.
To me that seems like the long term direction (a separate rf python package that extends tidy3d, but tidy3d proper can be independent of it)
There was a problem hiding this comment.
MicrowaveModeSpec makes sense but then I need to create new classes that accept it, which will lead to MicrowaveModeData/MicrowaveModeSolverData/MicrowaveModeMonitor/MicrowaveModeSolverMonitor/MicrowaveModeSimulation versions?
Or do I allow a ModeMonitor to accept both mode_specs but produce either ModeData or MicrowaveModeData depending on the spec?
There was a problem hiding this comment.
Ultimately, sounds like we should go this way in the long run @dmarek-flex though, and try to have a cleaner separation for RF. Happy to chat to scope this out if we want to do this for 2.10 too before getting to MicrowaveSimulation? This said it'd be good for the product council to discuss the extra effort this might take to start doing this separation in the RF product plans @tylerflex ?
There was a problem hiding this comment.
yea, it's a tough call, because we will start to have tons of classes, but I also feel it should happen eventually. Hopefully most of these could be very thin wrappers around the tidy3d objects that they inherit from?
Ideally we can get to a place where tidy3d just contains stuff for basic fdtd + photonics. and tidy3d.rf extends tidy3d and adds anything specific to microwave. I think we'll necessarily have to add more classes there but hopefully most things can be shared.
| "CurrentIntegralAxisAligned", | ||
| "CompositeCurrentIntegral", | ||
| "CurrentIntegralTypes", | ||
| "CustomCurrentIntegral2D", | ||
| "CustomPathIntegral2D", | ||
| "CustomVoltageIntegral2D", | ||
| "Custom2DCurrentIntegral", | ||
| "Custom2DPathIntegral", | ||
| "Custom2DVoltageIntegral", |
There was a problem hiding this comment.
Agree with the standardisation just got to check it is not in 2.9
add a ImpedanceSpec for controlling how characteristic impedance is calculated from modes refactor path integral names add doctests fix docstring
dmarek-flex
force-pushed
the
dmarek/add_impedance_to_mode_data
branch
from
a4c3c40 to
837a080
Compare
dmarek-flex
force-pushed
the
dmarek/add_impedance_to_mode_data
branch
from
837a080 to
c685b6b
Compare
add a ImpedanceSpec for controlling how characteristic impedance is calculated from modes
backend PR: https://github.com/flexcompute/compute/pull/2497
Greptile Overview
Updated On: 2025-09-23 19:17:19 UTC
Summary
This PR introduces a comprehensive impedance calculation system for microwave mode solvers. The implementation adds an
ImpedanceSpecfor controlling characteristic impedance calculations from modes, integrating seamlessly with the existing mode solver infrastructure.Key Changes:
ImpedanceSpecTypesunion withAutoImpedanceSpecandCustomImpedanceSpecclassesMicrowaveModeSpecto hold impedance specifications for each modeModeSpecwith optionalmicrowave_mode_specfield with proper validationImpedanceCalculatorwith flexible voltage/current path integral supportThe architecture properly separates concerns with automatic impedance calculation for simple cases and custom path integral specifications for advanced users. The implementation follows established patterns in the codebase and includes proper validation at multiple levels.
Confidence Score: 4/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant User participant ModeSpec participant MicrowaveModeSpec participant ImpedanceSpec participant ModeSolver participant ImpedanceCalculator participant PathIntegrals User->>ModeSpec: Create mode spec with microwave_mode_spec ModeSpec->>MicrowaveModeSpec: Validate impedance_spec count vs num_modes MicrowaveModeSpec->>ImpedanceSpec: Create AutoImpedanceSpec or CustomImpedanceSpec alt CustomImpedanceSpec ImpedanceSpec->>ImpedanceSpec: Validate voltage_spec or current_spec provided end User->>ModeSolver: solve() with ModeSpec ModeSolver->>PathIntegrals: make_path_integrals() for AutoImpedanceSpec PathIntegrals-->>ModeSolver: Return voltage/current integrals ModeSolver->>ImpedanceCalculator: Create calculator with integrals ImpedanceCalculator->>ImpedanceCalculator: Validate at least one integral provided ModeSolver->>ImpedanceCalculator: compute_impedance(em_field) ImpedanceCalculator->>ImpedanceCalculator: Check monitor data supported alt Both voltage and current integrals provided ImpedanceCalculator->>ImpedanceCalculator: Direct impedance = voltage/current else Only voltage integral ImpedanceCalculator->>ImpedanceCalculator: Use flux: impedance = voltage²/(2*flux) else Only current integral ImpedanceCalculator->>ImpedanceCalculator: Use flux: impedance = 2*flux/current² end ImpedanceCalculator-->>ModeSolver: Return impedance data array ModeSolver-->>User: Return ModeSolverData with impedance results