Refactor waveguide models to use sax[rf]#344
Merged
nikosavola merged 8 commits intomainfrom Mar 31, 2026
Merged
Conversation
Contributor
Reviewer's GuideRefactors the CPW and microstrip waveguide models to source all electromagnetic core calculations from sax.models.rf, updating the straight/straight_microstrip wrappers accordingly and adding sax[rf] (and scikit-rf) as dependencies. Sequence diagram for straight and straight_microstrip using sax.models.rfsequenceDiagram
participant qpdk_waveguides as qpdk_models_waveguides
participant qpdk_cpw as qpdk_models_cpw
participant sax_rf as sax_models_rf
rect rgb(235, 245, 255)
qpdk_waveguides->>qpdk_cpw: get_cpw_dimensions(cross_section)
qpdk_cpw-->>qpdk_waveguides: width, gap
qpdk_waveguides->>qpdk_cpw: get_cpw_substrate_params()
qpdk_cpw-->>qpdk_waveguides: h, t, ep_r
qpdk_waveguides->>sax_rf: coplanar_waveguide(f, length, width, gap, thickness=t, substrate_thickness=h, ep_r=ep_r)
sax_rf-->>qpdk_waveguides: SDict
end
rect rgb(245, 235, 255)
qpdk_waveguides->>sax_rf: microstrip(f, length, width, substrate_thickness=h, thickness=t, ep_r=ep_r, tand=tand)
sax_rf-->>qpdk_waveguides: SDict
end
Class diagram for updated qpdk RF waveguide model structureclassDiagram
class qpdk_models_cpw {
+cpw_ep_r_from_cross_section()
+cpw_epsilon_eff()
+cpw_thickness_correction()
+cpw_z0()
+cpw_z0_from_cross_section()
+get_cpw_dimensions(cross_section)
+get_cpw_substrate_params()
+microstrip_epsilon_eff()
+microstrip_thickness_correction()
+microstrip_z0()
+propagation_constant()
+transmission_line_s_params()
+cpw_parameters(width, gap)
}
class qpdk_models_waveguides {
+straight(f, length, cross_section)
+straight_microstrip(f, length, width, h, t, ep_r, tand)
+straight_shorted(f, length, cross_section)
}
class sax_models_rf {
+cpw_epsilon_eff()
+cpw_thickness_correction()
+cpw_z0()
+microstrip_epsilon_eff()
+microstrip_thickness_correction()
+microstrip_z0()
+propagation_constant()
+transmission_line_s_params()
+coplanar_waveguide(f, length, width, gap, thickness, substrate_thickness, ep_r)
+microstrip(f, length, width, substrate_thickness, thickness, ep_r, tand)
}
qpdk_models_cpw ..> sax_models_rf : reexports_functions
qpdk_models_waveguides ..> qpdk_models_cpw : uses_helpers
qpdk_models_waveguides ..> sax_models_rf : wraps_coplanar_waveguide_microstrip
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
dependencies
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| from qpdk.helper import deprecated | ||
| from qpdk.tech import material_properties | ||
|
|
||
| DEPRECATION_MSG = "Prefer cpw_parameters or the functions in qpdk.models.cpw for JAX-jittable analysis." |
Contributor
There was a problem hiding this comment.
The deprecation message and several docstrings in this file still reference qpdk.models.cpw, which was deleted in this PR. Anyone following the deprecated API guidance will hit an ImportError.
The message and docs on lines 41, 104, and 144 should be updated to point to sax.models.rf instead:
Suggested change
| DEPRECATION_MSG = "Prefer cpw_parameters or the functions in qpdk.models.cpw for JAX-jittable analysis." | |
| DEPRECATION_MSG = "Prefer cpw_parameters or the functions in sax.models.rf for JAX-jittable analysis." |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
straightandstraight_microstrip, the previous implementations explicitly converted length/geometry from µm to meters, whereas the new saxcoplanar_waveguide/microstripcalls receive rawlength,width,gap,thickness, andsubstrate_thicknessvalues—please confirm and, if necessary, fix the unit conventions so they match the expectations of the sax RF models. - The
DEPRECATION_MSGinqpdk/models/media.pystill referencesqpdk.models.cpw, which has been removed; update this message to point to the new recommended API (e.g., sax RF models) so it remains accurate. - The commented-out adjustments to
sax_stype_functionsintests/models/test_models_imported.pylook like temporary debugging scaffolding; either remove them or replace them with a more permanent mechanism if they are needed for maintaining the models registry consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `straight` and `straight_microstrip`, the previous implementations explicitly converted length/geometry from µm to meters, whereas the new sax `coplanar_waveguide`/`microstrip` calls receive raw `length`, `width`, `gap`, `thickness`, and `substrate_thickness` values—please confirm and, if necessary, fix the unit conventions so they match the expectations of the sax RF models.
- The `DEPRECATION_MSG` in `qpdk/models/media.py` still references `qpdk.models.cpw`, which has been removed; update this message to point to the new recommended API (e.g., sax RF models) so it remains accurate.
- The commented-out adjustments to `sax_stype_functions` in `tests/models/test_models_imported.py` look like temporary debugging scaffolding; either remove them or replace them with a more permanent mechanism if they are needed for maintaining the models registry consistency.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+150
to
+154
| # sax_stype_functions.add("straight") | ||
| # sax_stype_functions.add("straight_microstrip") | ||
| # sax_stype_functions.discard("coplanar_waveguide") | ||
| # sax_stype_functions.discard("microstrip") | ||
| # sax_stype_functions.discard("ellipk_ratio") |
Contributor
There was a problem hiding this comment.
These commented-out lines look like debugging artifacts left over from developing this test. They should be removed before merging.
Suggested change
| # sax_stype_functions.add("straight") | |
| # sax_stype_functions.add("straight_microstrip") | |
| # sax_stype_functions.discard("coplanar_waveguide") | |
| # sax_stype_functions.discard("microstrip") | |
| # sax_stype_functions.discard("ellipk_ratio") |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors QPDK’s CPW and microstrip waveguide modeling to rely on sax.models.rf implementations rather than maintaining local JAX-jittable CPW/microstrip formulations.
Changes:
- Replace
straight/straight_microstripimplementations with wrappers aroundsax.models.rfmodels. - Remove local CPW/microstrip analysis module (
qpdk/models/cpw.py) and its dedicated tests. - Update optional dependency set to use
sax[rf](and lockfile accordingly).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Switches models extra dependency from sax to sax[rf]. |
uv.lock |
Updates locked requirement metadata to include the rf extra for sax. |
qpdk/models/waveguides.py |
Re-implements CPW and microstrip straight models via sax.models.rf; updates cross-section helper import usage. |
qpdk/models/media.py |
Replaces CPW analysis imports with sax.models.rf equivalents. |
qpdk/models/__init__.py |
Re-exports RF analysis helpers from sax.models.rf instead of qpdk.models.cpw. |
tests/models/test_models_imported.py |
Adds commented-out adjustment lines (currently inactive) around model discovery assertion. |
qpdk/models/cpw.py |
Deleted (previous local CPW/microstrip analytic implementations). |
tests/models/test_cpw.py |
Deleted (tests for removed local CPW analysis and straight CPW JIT checks). |
tests/models/test_microstrip.py |
Deleted (tests for removed local microstrip analysis and straight_microstrip). |
Comments suppressed due to low confidence (1)
qpdk/models/media.py:24
DEPRECATION_MSG(and nearby docstrings) still referencesqpdk.models.cpwas the preferred JAX-jittable analysis module, but this PR deletesqpdk/models/cpw.pyand now imports these helpers fromsax.models.rf. Please update the message/docs to point to the new canonical location (e.g.,sax.models.rfor the re-exports inqpdk.models) to avoid broken guidance.
from qpdk.tech import material_properties
DEPRECATION_MSG = "Prefer cpw_parameters or the functions in qpdk.models.cpw for JAX-jittable analysis."
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
50
to
68
| def straight_microstrip( | ||
| f: sax.FloatArrayLike = DEFAULT_FREQUENCY, | ||
| length: sax.Float = 1000, | ||
| width: sax.Float = 10.0, | ||
| h: sax.Float = 500.0, | ||
| t: sax.Float = 0.2, | ||
| ep_r: sax.Float = 11.45, | ||
| tand: sax.Float = 0.0, | ||
| ) -> sax.SDict: | ||
| r"""S-parameter model for a straight microstrip transmission line. | ||
|
|
||
| Computes S-parameters analytically using the Hammerstad-Jensen | ||
| :cite:`hammerstadAccurateModelsMicrostrip1980` closed-form expressions | ||
| for effective permittivity and characteristic impedance, as described | ||
| in Pozar :cite:`m.pozarMicrowaveEngineering2012` (ch. 3, §3.8). | ||
| Conductor thickness corrections follow | ||
| Gupta et al. :cite:`guptaMicrostripLinesSlotlines1996` (§2.2.4). | ||
|
|
||
| All computation is done with pure-JAX functions | ||
| (see :mod:`qpdk.models.cpw`) so the model composes with ``jax.jit``, | ||
| ``jax.grad``, and ``jax.vmap``. | ||
|
|
||
| Args: | ||
| f: Array of frequency points in Hz. | ||
| length: Physical length in µm. | ||
| width: Strip width in µm. | ||
| h: Substrate height in µm. | ||
| t: Conductor thickness in µm (default 0.2 µm = 200 nm). | ||
| ep_r: Relative permittivity of the substrate (default 11.45 for Si). | ||
| tand: Dielectric loss tangent (default 0 — lossless). | ||
|
|
||
| Returns: | ||
| sax.SDict: S-parameters dictionary. | ||
| """ | ||
| f = jnp.asarray(f) | ||
| f_flat = f.ravel() | ||
|
|
||
| # Convert to SI (metres) | ||
| w_m = width * 1e-6 | ||
| h_m = h * 1e-6 | ||
| t_m = t * 1e-6 | ||
| length_m = jnp.asarray(length) * 1e-6 | ||
|
|
||
| # Effective permittivity (Hammerstad-Jensen) | ||
| ep_eff = microstrip_epsilon_eff(w_m, h_m, ep_r) | ||
|
|
||
| # Apply conductor thickness correction if t > 0 | ||
| _w_eff, ep_eff_t, z0_val = microstrip_thickness_correction( | ||
| w_m, h_m, t_m, ep_r, ep_eff | ||
| """S-parameter model for a straight microstrip line.""" | ||
| return _microstrip( | ||
| f=f, | ||
| length=length, | ||
| width=width, | ||
| substrate_thickness=h, | ||
| thickness=t, | ||
| ep_r=ep_r, | ||
| tand=tand, | ||
| ) |
Comment on lines
+12
to
+17
| ) | ||
| from sax.models.rf import ( | ||
| electrical_open, | ||
| electrical_short, | ||
| ) | ||
| from sax.models.rf import ( |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Resolve conflicts in pyproject.toml and uv.lock - Restore and update CPW and microstrip models in qpdk/models/cpw.py - Align waveguide models with main's analytical implementations - Update imports in qpdk/models/__init__.py to fix ImportError
github-actions
Bot
removed
tests
Member
Author
|
@copilot could you refactor the coplanar waveguide, microstrip and related elementary Sax models to use the versions from sax.models.rf (in latest Sax version). For example, you should likely wrap the models like |
- Replace local CPW/microstrip implementations in cpw.py with re-exports from sax.models.rf - Wrap sax.models.rf.coplanar_waveguide in straight(), extracting width/gap from cross_section and substrate params from PDK layer stack - Wrap sax.models.rf.microstrip in straight_microstrip() - Consolidate sax.models.rf imports into single block in waveguides.py Agent-Logs-Url: https://github.com/gdsfactory/quantum-rf-pdk/sessions/ab5b541c-1ae0-451b-8a90-ef62bdd60941 Co-authored-by: nikosavola <7860886+nikosavola@users.noreply.github.com>
Contributor
Done in dab1078. Refactored |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In both
straightandstraight_microstrip, the old implementations explicitly converted geometry and length from µm to metres before calling the EM routines, whereas the new wrappers pass the raw values through tosax.models.rf; please double‑check and, if needed, normalise units so the upstream models still see SI units. - The previous
straight/straight_microstripimplementations built a partial S‑matrix and then enforced reciprocity viasax.reciprocal, while the new versions return the S‑dict fromsax.models.rfdirectly; confirm that the upstream models already produce symmetric/reciprocal 2‑port S‑parameters with the expected port naming to avoid behavioral changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both `straight` and `straight_microstrip`, the old implementations explicitly converted geometry and length from µm to metres before calling the EM routines, whereas the new wrappers pass the raw values through to `sax.models.rf`; please double‑check and, if needed, normalise units so the upstream models still see SI units.
- The previous `straight`/`straight_microstrip` implementations built a partial S‑matrix and then enforced reciprocity via `sax.reciprocal`, while the new versions return the S‑dict from `sax.models.rf` directly; confirm that the upstream models already produce symmetric/reciprocal 2‑port S‑parameters with the expected port naming to avoid behavioral changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors CPW and microstrip waveguide models to utilize
sax[rf]instead of local JAX-jittable implementations.qpdk/models/cpw.py(cpw_epsilon_eff,cpw_z0,cpw_thickness_correction,microstrip_epsilon_eff,microstrip_z0,microstrip_thickness_correction,propagation_constant,transmission_line_s_params) with re-exports fromsax.models.rf. Layout-to-model helpers (get_cpw_dimensions,cpw_parameters, etc.) are retained.sax.models.rf.coplanar_waveguideinstraight(), extracting width/gap from thecross_sectionand substrate parameters from the PDK layer stack.sax.models.rf.microstripinstraight_microstrip(), mapping qpdk parameter names to the upstream model.sax.models.rfimports into a single grouped import block inwaveguides.py.pyproject.tomlto includesax[rf].Drafted until flaport/sax#100 is in a release
Summary by Sourcery
Refactor CPW and microstrip waveguide models to delegate electromagnetic calculations to shared sax[rf] implementations while preserving qpdk-specific layout-to-model helpers.
Enhancements:
Build: