-
Notifications
You must be signed in to change notification settings - Fork 65
FXC-3517 add impedance calculations to mode solver #2837
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
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.
28 files reviewed, 2 comments
ceccb9e to
18ced37
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/geometry/utils.pytidy3d/components/microwave/base.pytidy3d/components/microwave/path_integrals/factory.pytidy3d/components/microwave/path_integrals/integrals/base.pytidy3d/components/microwave/path_integrals/integrals/current.pytidy3d/components/mode/mode_solver.pytidy3d/plugins/microwave/auto_path_integrals.pytidy3d/plugins/microwave/custom_path_integrals.pytidy3d/plugins/microwave/impedance_calculator.pytidy3d/plugins/microwave/path_integrals.py |
6996084 to
290484a
Compare
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.
Most went through code structures, and the structure looks in good shape. Some minor comments:
dbochkov-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.
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
7ad7a12 to
2727e2f
Compare
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 |
daquinteroflex
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.
Looks pretty good @dmarek-flex ! Just a few questions on very few API design considerations
837a080 to
c685b6b
Compare
bee9d40 to
008ab9f
Compare
|
Alright lets have another go @greptileai. Also since @daquinteroflex is away, can @tylerflex or @yaugenst-flex take a look at this refactoring of the |
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.
Additional Comments (1)
-
tidy3d/plugins/microwave/auto_path_integrals.py, line 32-34 (link)logic: Parameter descriptions don't match parameter names -
lumped_elementandgriddescriptions are incorrectContext Used: Rule from
dashboard- Keep docstrings and comments synchronized with code changes to ensure they are always accurate and n... (source)
63 files reviewed, 9 comments
b2458d6 to
40bcc17
Compare
We should figure out a plan regarding the RF/Tidy3D separation and organization long term. @weiliangjin2021 just for a sense, here are some related discussions #2837 (comment) My feeling is that the best long term solution is a complete separation of RF from the main tidy3d components. Meaning perhaps a But for this to be a reality, we would need to (at least):
Could you guys think about this a bit in terms of requirements from your side and any preferences? @weiliangjin2021 let's discuss together with yannick and Lucas at the tech council meeting monday to get a general feeling because the same issues arise in photonforge. |
|
I just started looking at this: One idea occurred to me, which is that if we eventually do want to import While underlying things would still be all mixed without any changes, this would at least give us an ability to start crafting the user-facing side of the package. note sure if worth doing here since this is a needed feature, but wanted to make a note. |
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.
So we've decided to refactor this in a separate PR into tidy3d.rf, for now feel free to merge but won't be like this for rc3
Will do another review pass tomorrow my morning.
daquinteroflex
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.
Just the few minor comments
27d9f00 to
9aa1b9b
Compare
|
Just made some final tests with the deployed solver and everything looks good. I decided to submit the docs changes in a different PR so that we can take our time with that #2897 @dbochkov-flexcompute Let me know if you approve of the changes regarding the license warning. |
9aa1b9b to
7a5edfc
Compare
add a ImpedanceSpec for controlling how characteristic impedance is calculated from modes add MicrowaveModeMonitor and MicrowaveModeSolverMonitor that accepts the new MicrowaveModeSpec BREAKING CHANGE: changed path integral class names in plugins/microwave
7a5edfc to
c8f2694
Compare
add a ImpedanceSpec for controlling how characteristic impedance is calculated from modes
backend PR: https://github.com/flexcompute/compute/pull/2497
- [ ] Finalize docs including the new Microwave components somewhere (monitors/mode_specs/path_specs)Now tracking here
Greptile Overview
Updated On: 2025-10-07 20:11:19 UTC
Summary
This PR introduces a comprehensive impedance calculation system for microwave mode solvers in Tidy3D. The implementation adds the ability to calculate characteristic impedance from electromagnetic modes, which is essential for microwave and RF transmission line analysis.Key architectural changes:
ImpedanceSpecTypesunion withAutoImpedanceSpecfor automatic impedance calculation andCustomImpedanceSpecfor user-defined voltage/current path integralsMicrowaveModeSpecextending the base mode specification with impedance calculation capabilities for each modeModeSpec,ModeSolver, and related classes to support microwave specifications while maintaining backward compatibilityMicrowaveModeMonitorandMicrowaveModeSolverMonitorfor microwave-specific data collectionMicrowaveModeDataandMicrowaveModeSolverDataclasses that include transmission line characteristics (impedance, voltage/current coefficients)Implementation highlights:
ModePlaneAnalyzerMicrowaveBaseModelbase class for consistent RF licensing and functionalityVoltageIntegralAxisAligned→AxisAlignedVoltageIntegral)The system integrates seamlessly with existing Tidy3D workflows - users can continue using standard mode specifications for basic electromagnetic analysis while accessing advanced impedance calculations through the new microwave specifications. The architecture properly separates automatic impedance calculation for common cases from custom path integral specifications for complex geometries.
PR Description Notes:
Important Files Changed
Changed Files
tidy3d/components/microwave/path_integrals/impedance_spec.pytidy3d/components/microwave/microwave_mode_spec.pytidy3d/plugins/microwave/impedance_calculator.pytidy3d/components/mode_spec.pytidy3d/components/microwave/path_integrals/mode_plane_analyzer.pytidy3d/components/mode/mode_solver.pytidy3d/components/microwave/path_integrals/base_spec.pytidy3d/components/microwave/path_integrals/voltage_spec.pytidy3d/components/microwave/path_integrals/current_spec.pytidy3d/components/microwave/monitor.pytidy3d/components/microwave/data/monitor_data.pytidy3d/components/microwave/base.pytidy3d/components/simulation.pytidy3d/components/geometry/utils.pytidy3d/plugins/microwave/path_integrals.pytidy3d/plugins/microwave/custom_path_integrals.pydocs/api/microwave/microwave_migration.rsttidy3d/__init__.pyCHANGELOG.mdConfidence score: 3/5
tidy3d/components/mode_spec.pyfor the missing closing quote in the validation error messageContext used:
Rule from
dashboard- Do not use markdown formatting in exception or warning messages; use single quotes to highlight vari... (source)Rule from
dashboard- Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)