feat: add new cells and GSG cross-section#46
Open
joamatab wants to merge 3 commits into
Open
Conversation
…cross-section Add commonly used wrapper cells and a die_phix_rf floorplan with RF pads.
Reviewer's GuideAdds wrapper cells for common photonic components (straight waveguide, Euler bends, tapers, text, and pads), introduces a parametrized RF-focused die floorplan with edge couplers and RF pads, and defines a new ground-signal-ground (GSG) cross-section used by the new RF pad cell. 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 left some high level feedback:
- In
double_linear_inverse_taper_mirror,gf.functions.mirror(c)uses the default mirror axis and component naming; consider passing the mirror axis explicitly (e.g.,p1/p2) and setting a distinct name to guarantee the taper is mirrored as described in the docstring and to avoid potential name clashes with the original component. - The
text_rectangularwrapper hardcodesposition=(0.0, 0.0); exposingpositionas a parameter and forwarding it togf.c.text_rectangularwould make the wrapper more generally usable and consistent with the underlying API. - For the new
gsgcross-section, you might want to accept aLayerSpecand default toLAYER.TL(as in thepadcell) instead of a plain string to keep layer specification consistent across the tech and cells.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `double_linear_inverse_taper_mirror`, `gf.functions.mirror(c)` uses the default mirror axis and component naming; consider passing the mirror axis explicitly (e.g., `p1`/`p2`) and setting a distinct name to guarantee the taper is mirrored as described in the docstring and to avoid potential name clashes with the original component.
- The `text_rectangular` wrapper hardcodes `position=(0.0, 0.0)`; exposing `position` as a parameter and forwarding it to `gf.c.text_rectangular` would make the wrapper more generally usable and consistent with the underlying API.
- For the new `gsg` cross-section, you might want to accept a `LayerSpec` and default to `LAYER.TL` (as in the `pad` cell) instead of a plain string to keep layer specification consistent across the tech and cells.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 several new components and cross-sections to the LNOI400 PDK, including wrappers for straight waveguides, Euler bends, mirrored double linear inverse tapers, pads, and Phix RF die frames, as well as a new GSG (Ground-Signal-Ground) cross-section. Feedback on these changes highlights a runtime error in the GSG cross-section where port_names and port_types are incorrectly passed to Section instead of CrossSection. Additionally, it is recommended to use LayerSpec and LAYER.TL instead of raw strings for layer parameters, and to add a return type annotation to the pad_gsg cell for consistency.
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.
Comment on lines
+286
to
+297
| sections = [ | ||
| Section( | ||
| width=width, | ||
| layer=layer, | ||
| offset=0, | ||
| port_names=port_names_electrical, | ||
| port_types=port_types_electrical, | ||
| ), | ||
| Section(width=width, layer=layer, offset=-gap - width), | ||
| Section(width=width, layer=layer, offset=+gap + width), | ||
| ] | ||
| return CrossSection(sections=tuple(sections), radius=radius or 3 * width + 2 * gap) |
There was a problem hiding this comment.
In gdsfactory, the Section class does not accept port_names and port_types arguments. Passing them to Section will result in a validation/runtime error. Instead, these arguments should be passed to the CrossSection constructor.
Suggested change
| sections = [ | |
| Section( | |
| width=width, | |
| layer=layer, | |
| offset=0, | |
| port_names=port_names_electrical, | |
| port_types=port_types_electrical, | |
| ), | |
| Section(width=width, layer=layer, offset=-gap - width), | |
| Section(width=width, layer=layer, offset=+gap + width), | |
| ] | |
| return CrossSection(sections=tuple(sections), radius=radius or 3 * width + 2 * gap) | |
| sections = [ | |
| Section( | |
| width=width, | |
| layer=layer, | |
| offset=0, | |
| ), | |
| Section(width=width, layer=layer, offset=-gap - width), | |
| Section(width=width, layer=layer, offset=+gap + width), | |
| ] | |
| return CrossSection( | |
| sections=tuple(sections), | |
| radius=radius or 3 * width + 2 * gap, | |
| port_names=port_names_electrical, | |
| port_types=port_types_electrical, | |
| ) |
Comment on lines
+277
to
+283
| def gsg( | ||
| trace_width: float = 100, | ||
| layer: str = "TL", | ||
| gap: float = 50, | ||
| radius: float | None = None, | ||
| **kwargs, | ||
| ) -> CrossSection: |
There was a problem hiding this comment.
For consistency and robustness, use LayerSpec and LAYER.TL instead of a raw string "TL" for the layer parameter. This ensures that the layer is correctly resolved using the PDK's layer map.
Suggested change
| def gsg( | |
| trace_width: float = 100, | |
| layer: str = "TL", | |
| gap: float = 50, | |
| radius: float | None = None, | |
| **kwargs, | |
| ) -> CrossSection: | |
| def gsg( | |
| trace_width: float = 100, | |
| layer: LayerSpec = LAYER.TL, | |
| gap: float = 50, | |
| radius: float | None = None, | |
| **kwargs, | |
| ) -> CrossSection: |
Comment on lines
+1569
to
+1570
| @gf.cell(tags=["die"]) | ||
| def pad_gsg(length: float = 100): |
There was a problem hiding this comment.
Add the return type annotation -> gf.Component to pad_gsg for consistency with other cell definitions in this file, and use a float literal 100.0 to match the type annotation of length.
Suggested change
| @gf.cell(tags=["die"]) | |
| def pad_gsg(length: float = 100): | |
| @gf.cell(tags=["die"]) | |
| def pad_gsg(length: float = 100.0) -> gf.Component: |
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
1 participant
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.
Summary
straight,bend_euler,double_linear_inverse_taper_mirror,text_rectangular,pad,pad_gsgdie_phix_rfdie floorplan with east/west edge couplers and north/south RF padsgsg(ground-signal-ground) cross-section intech.pyTest plan
make testto verify no regressionsbuild_cellsand inspect GDS outputSummary by Sourcery
Add new photonic and RF layout cells and a corresponding ground-signal-ground cross-section to support edge-coupled dies with RF pad layouts.
New Features: