merge IIC-JKU new passive rf components#184
Conversation
As stated in the docs is should be ", 0)"
changed to use the layer definitons from tech.py and also moved the design rules to tech.py to have all definitions in one place
This reverts commit ebad20d.
Copy Paste IHP PCell code and rework so it works in the process the via array function was changed so via stack wont work as intended
mdmaas
requested review from
Alex-l-r,
ThomasPluck,
cdaunt,
joamatab,
nikosavola,
sheron-lian and
thanojo
as code owners
Reviewer's GuideIntroduces a new RF passive device module and extends waveguide primitives to support impedance-aware transmission line synthesis (including bends, couplers, and filters) while wiring these new cells into the IHP PDK, all built on top of the upstream gdsfactory/IHP core without bringing in pycell-dependent code. Flow diagram for tline width/Z0 resolution and layoutflowchart TD
A[tline called] --> B{width provided?}
B -- yes --> C1[_calculate_Z0_from_width]
B -- no --> C2{Z0 provided?}
C2 -- no --> E[raise ValueError: Provide either width or Z0]
C2 -- yes --> D[_calculate_width_from_Z0]
C1 --> F[build signal straight gf.c.straight]
D --> F
F --> G{ground_cross_section is list?}
G -- yes --> H[build ground_low & ground_high gf.c.straight]
G -- no --> I[build single ground gf.c.straight]
H --> J[add ports from signal]
I --> J
J --> K[return Component]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Several helpers (e.g.
_get_stack_geometry,_calculate_effective_dielectric_constant,_calculate_width_from_Z0,_calculate_Z0_from_width) assumeCrossSectionSpecis a string with a specific naming convention and usesplit('_')[0], which will break for non-string specs or different naming; consider normalizing viagf.get_cross_section(...)and layer metadata instead of relying on string parsing and implicit ordering oflayer_stack().layers. - The width/Z0 argument validation and conversion logic is duplicated across multiple tline-related cells (
tline,tline_bend_*,tline_bend_s,tline_corner); factoring this into a shared helper (e.g.resolve_width_and_Z0(...)) would reduce repetition and keep behavior consistent when updating the impedance/width formulas.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several helpers (e.g. `_get_stack_geometry`, `_calculate_effective_dielectric_constant`, `_calculate_width_from_Z0`, `_calculate_Z0_from_width`) assume `CrossSectionSpec` is a string with a specific naming convention and use `split('_')[0]`, which will break for non-string specs or different naming; consider normalizing via `gf.get_cross_section(...)` and layer metadata instead of relying on string parsing and implicit ordering of `layer_stack().layers`.
- The width/Z0 argument validation and conversion logic is duplicated across multiple tline-related cells (`tline`, `tline_bend_*`, `tline_bend_s`, `tline_corner`); factoring this into a shared helper (e.g. `resolve_width_and_Z0(...)`) would reduce repetition and keep behavior consistent when updating the impedance/width formulas.
## Individual Comments
### Comment 1
<location path="ihp/cells/waveguides.py" line_range="85" />
<code_context>
+ for i in range(signal_idx + 1, len(keys))
+ )
+
+ e_eff = e_r * (1-exp(-1.55*(h_p+t+h_above)/h_p))
+
+ return e_eff
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against division by zero when stack_height h_p is zero in effective dielectric calculation.
`_get_stack_geometry` can return `h_p == 0` when the signal and ground layers are adjacent (or if layer ordering changes), which will raise a `ZeroDivisionError` in `(h_p + t + h_above) / h_p`. Please handle `h_p <= 0` explicitly (e.g., fallback to `e_r` or use a small epsilon) before using it in this expression.
</issue_to_address>
### Comment 2
<location path="ihp/cells/rf_devices.py" line_range="805-811" />
<code_context>
+ return c
+
+
+def _butterworth_prototype(N: int) -> list[float]:
+ """Return Butterworth lowpass prototype element values g_0 … g_{N+1}."""
+ # g = [1.0]
+ g = []
+ for k in range(1, N + 1):
+ g.append(2 * sin((2 * k - 1) * pi / (2 * N)))
+ g.append(1.0)
+ return g
+
</code_context>
<issue_to_address>
**suggestion:** Butterworth prototype helper’s contract/comments don’t clearly match how g-values are later used.
The docstring claims this returns `g_0 … g_{N+1}`, but the function actually returns `[g1, …, gN, 1.0]` with no leading `g0`. Callers like `coupled_line_bandpass_filter` then treat this as `[g1…gN, g_{N+1}]` and prepend `1.0` for `g0`. This contract mismatch makes future misuse (e.g., double‑prepending `g0`) likely. Consider either returning `[1.0, g1, …, gN, 1.0]` and updating callers, or revising the docstring to document the current `[g1…gN, g_{N+1}]` contract and that `g0` is added by the caller.
Suggested implementation:
```python
def _estimate_rppd_length(resistance_ohm: float, width_um: float, sheet_res_ohm_per_sq: float = 300.0) -> float:
"""Estimate rppd length from target resistance using upstream resistor model constants."""
def _butterworth_prototype(N: int) -> list[float]:
"""Return Butterworth lowpass prototype element values g_0 … g_{N+1}."""
g: list[float] = [1.0] # g0
for k in range(1, N + 1):
g.append(2 * sin((2 * k - 1) * pi / (2 * N)))
g.append(1.0) # g_{N+1}
return g
```
The updated helper now returns `[g0, g1, …, gN, g_{N+1}]`. Anywhere `_butterworth_prototype` is used and `1.0` is manually prepended (e.g., patterns like `[1.0] + _butterworth_prototype(N)` or `g = [1.0, * _butterworth_prototype(N)]` inside functions such as `coupled_line_bandpass_filter`), those manual `g0` prepends must be removed. Callers should instead use the list returned by `_butterworth_prototype(N)` directly as `[g0, …, g_{N+1}]`.
</issue_to_address>
### Comment 3
<location path="ihp/cells/rf_devices.py" line_range="42" />
<code_context>
+ Z0: Target characteristic impedance (ohms).
+ e_r: Relative permittivity of the substrate. Defaults to 4.1 for silicon dioxide.
+ """
+ wave_length = 3e8 / frequency * 1e6
+
+ c = gf.Component()
</code_context>
<issue_to_address>
**suggestion:** Use a consistent speed-of-light source and units across RF building blocks.
This function hardcodes `c = 3e8`, while others (e.g. `directional_coupler`, `quarter_wave_transformer`, filters) use `scipy.constants.c`. Please switch to `scipy.constants.c` here as well so wavelength/center‑frequency calculations remain consistent and easier to verify across the codebase.
Suggested implementation:
```python
import gdsfactory as gf
from scipy import constants
```
```python
wave_length = constants.c / frequency * 1e6
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for i in range(signal_idx + 1, len(keys)) | ||
| ) | ||
|
|
||
| e_eff = e_r * (1-exp(-1.55*(h_p+t+h_above)/h_p)) |
There was a problem hiding this comment.
issue (bug_risk): Guard against division by zero when stack_height h_p is zero in effective dielectric calculation.
_get_stack_geometry can return h_p == 0 when the signal and ground layers are adjacent (or if layer ordering changes), which will raise a ZeroDivisionError in (h_p + t + h_above) / h_p. Please handle h_p <= 0 explicitly (e.g., fallback to e_r or use a small epsilon) before using it in this expression.
| def _butterworth_prototype(N: int) -> list[float]: | ||
| """Return Butterworth lowpass prototype element values g_0 … g_{N+1}.""" | ||
| # g = [1.0] | ||
| g = [] | ||
| for k in range(1, N + 1): | ||
| g.append(2 * sin((2 * k - 1) * pi / (2 * N))) | ||
| g.append(1.0) |
There was a problem hiding this comment.
suggestion: Butterworth prototype helper’s contract/comments don’t clearly match how g-values are later used.
The docstring claims this returns g_0 … g_{N+1}, but the function actually returns [g1, …, gN, 1.0] with no leading g0. Callers like coupled_line_bandpass_filter then treat this as [g1…gN, g_{N+1}] and prepend 1.0 for g0. This contract mismatch makes future misuse (e.g., double‑prepending g0) likely. Consider either returning [1.0, g1, …, gN, 1.0] and updating callers, or revising the docstring to document the current [g1…gN, g_{N+1}] contract and that g0 is added by the caller.
Suggested implementation:
def _estimate_rppd_length(resistance_ohm: float, width_um: float, sheet_res_ohm_per_sq: float = 300.0) -> float:
"""Estimate rppd length from target resistance using upstream resistor model constants."""
def _butterworth_prototype(N: int) -> list[float]:
"""Return Butterworth lowpass prototype element values g_0 … g_{N+1}."""
g: list[float] = [1.0] # g0
for k in range(1, N + 1):
g.append(2 * sin((2 * k - 1) * pi / (2 * N)))
g.append(1.0) # g_{N+1}
return gThe updated helper now returns [g0, g1, …, gN, g_{N+1}]. Anywhere _butterworth_prototype is used and 1.0 is manually prepended (e.g., patterns like [1.0] + _butterworth_prototype(N) or g = [1.0, * _butterworth_prototype(N)] inside functions such as coupled_line_bandpass_filter), those manual g0 prepends must be removed. Callers should instead use the list returned by _butterworth_prototype(N) directly as [g0, …, g_{N+1}].
| Z0: Target characteristic impedance (ohms). | ||
| e_r: Relative permittivity of the substrate. Defaults to 4.1 for silicon dioxide. | ||
| """ | ||
| wave_length = 3e8 / frequency * 1e6 |
There was a problem hiding this comment.
suggestion: Use a consistent speed-of-light source and units across RF building blocks.
This function hardcodes c = 3e8, while others (e.g. directional_coupler, quarter_wave_transformer, filters) use scipy.constants.c. Please switch to scipy.constants.c here as well so wavelength/center‑frequency calculations remain consistent and easier to verify across the codebase.
Suggested implementation:
import gdsfactory as gf
from scipy import constants wave_length = constants.c / frequency * 1e6 ![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new rf_devices.py module containing various RF components (such as couplers, power dividers, and filters) and extends waveguides.py with transmission line primitives and analytical impedance calculations. The review feedback identifies several critical layout and routing issues, including short-circuits in the directional coupler, asymmetry in the Wilkinson power divider, and an incorrect circumference calculation due to a commented-out resistor. Additionally, there are potential runtime errors from unsafe log arguments and scipy.constants namespace access, as well as a violation of gdsfactory best practices regarding the use of .copy() on cell components.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| corner_left = c.add_ref(tline_corner( | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| Z0=Z0, | ||
| )) | ||
|
|
||
| corner_left.connect("e1", coupled_lines.ports["e4"]) | ||
| connection_port4 = c.add_ref(tline( | ||
| length=connection_length, | ||
| Z0=Z0, | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| )) | ||
|
|
||
| corner_right = c.add_ref(tline_corner( | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| Z0=Z0, | ||
| )) | ||
| connection_port4.connect("e1", corner_left.ports["e2"]) | ||
|
|
||
| corner_right.connect("e1", coupled_lines.ports["e3"]) | ||
| connection_port3 = c.add_ref(tline( | ||
| length=connection_length, | ||
| Z0=Z0, | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| )) | ||
| connection_port3.connect("e1", corner_right.ports["e4"]) | ||
|
|
There was a problem hiding this comment.
In directional_coupler, the coupled ports (ports 3 and 4) are routed upwards (towards positive Y) because the corners (corner_left and corner_right) are not properly mirrored/oriented. This causes connection_port3 and connection_port4 to cross and short-circuit with the main transmission lines at the top. The corners must be oriented so that the coupled ports point downwards (away from the main line).
| corner_output_p2 = c.add_ref(tline_corner( | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| Z0=Z0*sqrt(2) | ||
| )) | ||
| corner_output_p2.connect( | ||
| "e1", branch_right_down.ports["e2"] | ||
| ) | ||
|
|
||
| corner_output_p3 = c.add_ref(tline_corner( | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| Z0=Z0*sqrt(2) | ||
| )) | ||
| corner_output_p3.connect( | ||
| "e2", branch_right_up.ports["e2"] | ||
| ) | ||
|
|
||
| connection_out_p2 = c.add_ref(tline( | ||
| length=connection_length, | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| width=width_Z0, | ||
| )) | ||
|
|
||
| connection_out_p2.connect( | ||
| "e1", corner_output_p2.ports["e2"], allow_width_mismatch=True | ||
| ) | ||
| connection_out_p2.movey(width_Z0/2 - width_Z0_sqrt2/2) | ||
|
|
||
| connection_out_p3 = c.add_ref(tline( | ||
| length=connection_length, | ||
| signal_cross_section=signal_cross_section, | ||
| ground_cross_section=ground_cross_section, | ||
| width=width_Z0, | ||
| )) | ||
|
|
||
| connection_out_p3.connect( | ||
| "e1", corner_output_p3.ports["e1"], allow_width_mismatch=True | ||
| ) | ||
| connection_out_p3.movey(-(width_Z0/2 - width_Z0_sqrt2/2)) | ||
|
|
There was a problem hiding this comment.
The Wilkinson power divider layout is completely asymmetric because the output corners (corner_output_p2 and corner_output_p3) are not mirrored/rotated symmetrically. This causes the output ports to point in different directions (one up, one left) and results in an electrically unbalanced design. Additionally, movey is used on connection_out_p2 which is oriented along Y, causing a gap or overlap instead of a lateral edge alignment.
| if shape == "C": | ||
|
|
||
| # Calculate the circumference of the square | ||
| circumference = quater_wave_length * 2 + length_R |
There was a problem hiding this comment.
The isolation resistor rppd is currently commented out (lines 616-622), but its estimated length length_R is still included in the circumference calculation. This artificially increases the branch lengths of the Wilkinson divider, shifting its operating frequency lower than the target frequency. If the resistor is not placed, length_R should be excluded from the circumference.
| circumference = quater_wave_length * 2 + length_R | |
| circumference = quater_wave_length * 2 |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…maas/IHP into experiment/massive-pr-attempt # Conflicts: # ihp/cells/rf_devices.py
4 participants
This branch started from a fork state updated-IHP that was significantly diverged from upstream:
updated-IHP/mainwas 382 commits behind and 144 commits ahead ofgdsfactory/IHP main.Given that divergence, this PR intentionally did not adopt the fork core wholesale. Instead, it applied a core-first integration strategy:
gdsfactory/IHPcore implementations as the baseline.sys.path/sg13g2_pycell_lib/cniruntime assumptions.What this PR adds
ihp/cells/rf_devices.pyihp/cells/__init__.pyihp/cells/waveguides.pyValidation performed
tlinebranch_line_couplerwilkinson_power_dividerdirectional_couplerquarter_wave_transformerNotes
Summary by Sourcery
Add new RF passive transmission-line primitives and higher-level RF components for the IHP PDK.
New Features:
Enhancements: