feat(gf180mcu): add logical and geometric electrical pins for SPICE export - #156
Open
das-dias wants to merge 36 commits into
Open
feat(gf180mcu): add logical and geometric electrical pins for SPICE export#156das-dias wants to merge 36 commits into
das-dias wants to merge 36 commits into
Conversation
…to PCells - Add pin layer definitions to layers.py (datatype 2, unused in this PDK): metal1_pin=(34,2), metal2_pin=(36,2), metal3_pin=(42,2), metal4_pin=(46,2), metal5_pin=(81,2), metaltop_pin=(53,2) - Add cells/_common.py with shared _add_pins helper that draws geometric pin rectangles (add_pin_rectangle_inside) on the correct pin layer using a metal drawing→pin layer map (metal1→metal1_pin, etc.) and registers logical DPins (create_pin) for all electrical ports, grouped by port name - Apply _add_pins(c) in all @gf.cell layout functions: cap_mim, cap_mos, diode (nd2ps, pd2nw), res, via_generator, via_stack, pcmpgr_gen (guardring) - Add tests/test_electrical_pins.py: 23 tests passing (1 skip: via_generator contact layer has no metal pin layer, by design)
…pins - Replace local _add_pins implementation with gdsfactory.add_electric_pins - Fixes per-port layer lookup bug (was using ports[0].layer for entire group) - Fix XOR test to skip pin layers (datatype 2) in addition to label layers Pin layers (datatype 2) are LVS metadata, not physical geometry. Reference GDS files from Magic do not contain pin geometry, so XOR tests skip these. **DEPENDS ON:** gdsfactory PR with add_electric_pins() function
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Reviewer's GuideAdds shared electrical pin infrastructure for GF180MCU PCells, wires key cells into it for SPICE-friendly export, adjusts tests/XOR comparison to tolerate new pin layers, and updates CI/dev workflows and dependencies. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
das-dias
marked this pull request as ready for review
July 29, 2026 20:18
das-dias
requested review from
MarkCunningham0410,
cdaunt,
joamatab,
kevinwguan and
thanojo
as code owners
July 29, 2026 20:18
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
das-dias
marked this pull request as draft
July 29, 2026 20:19
![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:
- The hardcoded numeric tuples in
_PIN_LAYER_MAPintest_electrical_pins.pyduplicate the layer information ingf180mcu.layers; consider deriving the pin-layer mapping fromLAYER/layerto avoid drift if the layer map changes. - Several diodes (
diode_nw2ps,diode_pw2dw,diode_dw2ps) call_add_pinsbut expose no electrical ports and are only mentioned in a comment as a known bug; it would be clearer to either fix their port definitions in this PR or mark them with an explicit TODO in the cell implementations rather than just in the test comment. - The release workflow is now fully driven via
workflow_dispatchand the sharedcut-releaseworkflow instead of tag pushes; if tag-based releases are still expected in your process, consider documenting or asserting the new entrypoint to avoid accidental changes to the release flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded numeric tuples in `_PIN_LAYER_MAP` in `test_electrical_pins.py` duplicate the layer information in `gf180mcu.layers`; consider deriving the pin-layer mapping from `LAYER`/`layer` to avoid drift if the layer map changes.
- Several diodes (`diode_nw2ps`, `diode_pw2dw`, `diode_dw2ps`) call `_add_pins` but expose no electrical ports and are only mentioned in a comment as a known bug; it would be clearer to either fix their port definitions in this PR or mark them with an explicit TODO in the cell implementations rather than just in the test comment.
- The release workflow is now fully driven via `workflow_dispatch` and the shared `cut-release` workflow instead of tag pushes; if tag-based releases are still expected in your process, consider documenting or asserting the new entrypoint to avoid accidental changes to the release flow.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
test_pdk_gds was calling remove_layers() directly on the cached kfactory cell, poisoning the cache for subsequent tests. test_geometric_pin_present then retrieved the same (now pin-layer-stripped) object and failed. Using dup() creates an independent copy so the cache is never mutated.
gdsfactoryplus is dev-only and not a core PDK dependency; the test_gfp jobs in the reusable workflow break when it isn't in the base export. Inline pre-commit and test_code directly to skip those jobs for now.
Set all _METAL_PIN_LAYERS values to None in _common.py so add_electric_pins registers logical electrical ports without drawing any pin polygon geometry. This aligns gf180mcu with every other PDK in the repo (logical-only mode). Consequences: - test_geometric_pin_present skipped with explicit reason message - test_pdk_gds no longer needs dup()+remove_layers(); reverted to simple difftest - _PIN_LAYERS list removed from test_components.py - gds_run snapshots updated to reflect cells without pin layer polygons
…ER names Replace hardcoded (layer, datatype) tuples with symbolic LAYER.metalX names from gf180mcu.layers. The dict comprehension maps each electrical drawing layer to None via kcl.layer(*s), aligning with the uniform Pattern B used across all PDKs in the repo.
…ss diodes - Add missing imports (AddPinFunction, add_pin_rectangle_inside) to _common.py - Complete pin_label_layer_map for all metal layers (metal1-metal5 + metaltop) - Aggregate e1+e2 ports into single "e" pin for via_generator and via_stack - Add explicit port_pin_mapping to cap_mim, cap_mos, res, pcmpgr_gen, diode_nd2ps, diode_pd2nw - Add anode/cathode ports to diode_nw2ps, diode_pw2dw, diode_dw2ps, sc_diode - Expand test suite: add new diode cells to CELL_NAMES, add test_expected_pin_names
Collaborator
Author
|
Test code / test / test_gfp_no_dev (pull_request) |
…n tests - Add LAYER.comp and LAYER.poly2 to pin_label_layer_map in _common.py - Add SKIP_DATATYPES constant in test_xor.py to skip pin (2), text (5), and label (10) datatypes during XOR comparison - Filter pin/label layers via dup() + remove_layers() in test_pdk_gds
das-dias
marked this pull request as ready for review
July 30, 2026 23:11
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Avoid
from ._common import *ingf180mcu/cells/__init__.pyand instead explicitly re-export the functions you want in the public API to prevent namespace pollution and accidental symbol exposure. - The pin/datatype skipping logic is duplicated between
tests/conftest.py(SKIP_DATATYPES) andtests/test_xor.py; consider centralizing these layer/datatype constants in a single module or fixture to keep the XOR and file-compare behavior consistent and easier to maintain. - In
_strip_pin_label_layersyou create aNamedTemporaryFilewithout closing it; using a context manager and explicitly closing the file before writing via KLayout (or switching toTemporaryDirectory/mkstemp) would avoid leaving open file descriptors around.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid `from ._common import *` in `gf180mcu/cells/__init__.py` and instead explicitly re-export the functions you want in the public API to prevent namespace pollution and accidental symbol exposure.
- The pin/datatype skipping logic is duplicated between `tests/conftest.py` (`SKIP_DATATYPES`) and `tests/test_xor.py`; consider centralizing these layer/datatype constants in a single module or fixture to keep the XOR and file-compare behavior consistent and easier to maintain.
- In `_strip_pin_label_layers` you create a `NamedTemporaryFile` without closing it; using a context manager and explicitly closing the file before writing via KLayout (or switching to `TemporaryDirectory`/`mkstemp`) would avoid leaving open file descriptors around.
## Individual Comments
### Comment 1
<location path="gf180mcu/cells/_common.py" line_range="69-82" />
<code_context>
+ pin_function: Function to draw each pin marker.
+ pin_type: Pin type string passed to create_pin().
+ """
+ if port_pin_mapping is not None:
+ by_name: dict[str, list] = {
+ pin_name: [component.ports[pn] for pn in port_names]
+ for pin_name, port_names in port_pin_mapping.items()
+ }
+ else:
+ by_name: dict[str, list] = defaultdict(list)
+ [
+ by_name[port.name].append(port)
+ for port in component.ports
</code_context>
<issue_to_address>
**suggestion:** Using a list comprehension purely for side effects harms readability and creates an unnecessary temporary list.
In the `else` branch, the comprehension is used only for its side effects and its result is discarded, which both hurts readability and allocates an unnecessary list. Please rewrite this as a simple loop:
```python
by_name: dict[str, list] = defaultdict(list)
for port in component.ports:
if port.port_type == "electrical":
by_name[port.name].append(port)
```
This preserves behavior while making the intent clearer and avoiding the extra allocation.
```suggestion
if port_pin_mapping is not None:
by_name: dict[str, list] = {
pin_name: [component.ports[pn] for pn in port_names]
for pin_name, port_names in port_pin_mapping.items()
}
else:
by_name: dict[str, list] = defaultdict(list)
for port in component.ports:
if port.port_type == "electrical":
by_name[port.name].append(port)
for name, ports in by_name.items():
```
</issue_to_address>
### Comment 2
<location path="gf180mcu/cells/_common.py" line_range="32-40" />
<code_context>
+
+
+# TODO: replace gdsfactory.add_pins:add_electric_pins in next gdsfactory release
+def _add_electric_pins(
+ component: Component,
+ port_pin_mapping: dict[str, list[str]] | None = None,
+ pin_layer_map: dict[LayerSpec, LayerSpec] | None = None,
+ pin_label_layer_map: dict[LayerSpec, LayerSpec] | None = None,
+ default_pin_layer: LayerSpec | None = None,
+ default_label_layer: LayerSpec | None = None,
+ pin_function: AddPinFunction = add_pin_rectangle_inside, # type: ignore[assignment]
+ pin_type: str = "DC",
+) -> None:
+ """Draw pin markers and register logical pins for all electrical ports.
</code_context>
<issue_to_address>
**suggestion:** The function signature could be tightened to use the existing `Component` type and improve type safety.
`Component` is already imported but `_add_pins` still uses an untyped `component` parameter. Annotating it as `Component` (to match `_add_electric_pins`) will improve type safety and make incorrect usage easier to detect:
```python
def _add_pins(component: Component, port_pin_mapping: dict[str, list[str]] | None = None) -> None:
...
```
Suggested implementation:
```python
def _add_pins(
component: Component,
port_pin_mapping: dict[str, list[str]] | None = None,
) -> None:
```
If `_add_pins` currently has a different parameter list or formatting (e.g. more parameters, different line breaks), adjust the SEARCH block to exactly match the existing `def _add_pins(...)` line(s), and then apply the same `component: Component` annotation in the REPLACE block. No other changes are required as `Component` is already imported in this module.
</issue_to_address>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
2 participants
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.
Closes doplaydo/pdks#45
Summary
add_electric_pins()to all PCells with electrical ports in gf180mcugf180mcu/cells/_common.pywith shared_add_pinspartialTest plan
tests/test_electrical_pins.pypassesSummary by Sourcery
Add shared electrical pin helper for GF180MCU PCells and wire it into diode, capacitor, resistor, guardring, and via cells, while updating tests and infrastructure to accommodate logical pins and pin layers.
New Features:
Bug Fixes:
Enhancements:
CI: