Fix order-dependent port renaming; close gdstk backend gaps - #102
Fix order-dependent port renaming; close gdstk backend gaps#102carloscl03 wants to merge 3 commits into
Conversation
|
Context on why this came up: #100's notebook check fails on Chasing that led here. With So this does not fix #100's check directly, but it makes switching the notebook |
4f83b7a to
16cf8a3
Compare
|
Measured what this unblocks, in case it helps prioritise it. The DRC and LVS workflows run the gdsfactory backend (CPython 3.10, gdsfactory 7.7 via On With this branch, the same command builds both and writes GDS and netlists — 0 errors. What is left is off-grid geometry ( So this is not only a fix for the alternative backend: together with #104 it is what would let the DRC/LVS workflows run on gdstk at all. Worth noting because on the runner that passed, the gdstk backend built the opamp cell in 55.5 s against 383 s on gdsfactory. |
|
Heads-up for whoever merges this and #104: the two branches conflict, and the obvious resolution is the wrong one. Both add to
Verified with the resolution applied, on top of
Same numbers as the tree these were originally measured on, so the resolution does not change behaviour. Happy to rebase this branch onto |
|
Another measurement that bears on this one, from the notebook CI on the fork. Running the tutorial suite on the gdstk backend, This matters beyond the one cell. The notebooks job is why six of the eight open PRs are red right now: two notebooks sit on the 180s per-cell limit and cross it depending on the runner, so the check flips on identical content. Moving the job to gdstk takes the suite from 660s to 158s and off the limit — but it needs this PR, #104 and #113 together. Without this one, Runs on this fork's CI, same container and runners: gdsfactory baseline · gdstk. Full table in #116. The |
Two notebooks sit on the 180s per-cell limit and cross it depending on
which runner the job lands on, so the check passes or fails on identical
content. `lvs-pin-filter` has four green runs and two red ones.
GLayout_Cells 290s pass (#112) / 372s error (#113)
glayout_opamp 136s pass (#112) / 198s error (#113)
The native backend builds these cells several times faster, which moves
them off the limit instead of raising it. Measured on this fork's CI --
same container, same runners -- with #102, #104 and #113 applied on both
sides, the only difference being this line:
gdsfactory gdstk
GLayout_Cells 187.8s ERROR 14.2s pass
glayout_opamp 126.4s pass 19.2s pass
5T_OTA_part2 90.8s pass 16.2s pass
test_bjt_custom_pattern 68.2s pass 11.4s pass
whole suite 660s 13/14 158s 14/14
Note the baseline already carries all three PRs and still fails: they are
what lets the notebooks run on gdstk at all, not what fixes the timeouts.
Depends on #102, #104 and #113. Without #113 `GLayout_Cells` raises in
`mimcap.py`; without #102 it raises in `Component.add`; without #104's
`add_ref(columns=)` the BJT notebook raises. This check stays red until
all three land.
…orts los dos, add_ref con arrays intacto
rename_component_ports renamed in place: pop the old key, insert the new one
on the same dict it was iterating. When a port renames onto a name another
port still holds, the guard
if namepair[0] in custom_comp.ports.keys():
silently skips it, so one of the two is dropped -- and which one survives
depends on dict insertion order.
rename_ports_by_orientation triggers this routinely: a mirrored port has
orientation 90 and legitimately renames _S -> _N onto a name that is still
queued. In diff_pair(gf180, width=3, fingers=4) that is 334 ports.
The result differs per backend. Same component, same port, orientation 90.0:
gdsfactory keeps bl_multiplier_0_source_S (90 deg is N, so this is wrong)
gdstk gives bl_multiplier_0_source_N (correct)
and add_df_labels then raises KeyError on the latter. It works under
gdsfactory by accident of ordering, not by construction.
Build the renamed mapping first, then swap it in. Order-independent.
diff_pair(gf180, width=3, fingers=4):
before 4530 ports gdstk: KeyError 'bl_multiplier_0_source_S'
after 4864 ports both backends identical, lookup succeeds
The 334 extra ports are the ones that were being dropped.
With GLAYOUT_BACKEND=gdstk the opamp failed on gf180. Four distinct gaps,
found by fixing one and hitting the next:
1. Component.add() rejected a bare Component. gdsfactory accepts one and
references it implicitly; several composite cells rely on that.
2. diff_pair_stackedcmirror imported rectangle straight from gdsfactory
(`from gdsfactory.components.rectangle import rectangle as _rect`),
bypassing the backend abstraction, so the active backend could not
reference the result. The module already imports the backend's rectangle
at module level.
3. ComponentReference.info did not reach the referenced component. mimcap_array
writes info['netlist'] on the Component while opamp_twostage reads it back
off the reference returned by '<<' -- gdsfactory exposes the parent's info
through the reference, so delegate instead of holding an empty dict.
4. Component.ref() took no arguments. opamp.py calls ref(position=...);
added position/rotation/x_reflection to match gdsfactory.
opamp(gf180, ...) with GLAYOUT_BACKEND=gdstk, same parameters as
GLayout_Cells.ipynb cell 14:
gdsfactory backend 383.0 s
gdstk backend 55.5 s 99090 ports
Requires the port rename fix in the previous commit; without it the run stops
earlier with KeyError: 'bl_multiplier_0_source_S'.
gdsfactory offers it on Component and the tutorials use it to inspect a cell while building it. Without it a notebook that runs on the other backend dies with AttributeError halfway through.
16cf8a3 to
60fad74
Compare
|
Rebased onto current Worth flagging what that surfaced. Against Those three cells were marked as known-broken; the port-renaming fix and the On merging alongside #104There is no conflict with
Verified on the fork's CI with that resolution plus #100/#103/#113/#114: the tutorial suite runs 14/14 in 156s. Happy to push the resolved merge here the moment #104 lands, whichever order suits you. |
Two commits. The first is a real bug in
port_utilsthat affects the defaultbackend too; the second unblocks
GLAYOUT_BACKEND=gdstkfor the opamp.Renaming ports depends on dict order
rename_component_portsrenames in place — pops the old key and inserts thenew one on the same dict it just iterated:
When a port renames onto a name another port still holds, that guard silently
skips it and one of the two is dropped. Which one survives depends on insertion
order.
rename_ports_by_orientationtriggers this routinely: a mirrored port hasorientation 90 and legitimately renames
_S->_Nonto a name still queued.In
diff_pair(gf180, width=3, fingers=4)that is 334 ports.Same component, same port, orientation 90.0:
bl_multiplier_0_source_S(90° is N, so this is wrong)bl_multiplier_0_source_N(correct)add_df_labelsthen raisesKeyErroron the latter. It works under gdsfactoryby accident of ordering, not by construction.
Fix builds the renamed mapping first, then swaps it in.
The 334 extra ports are the ones that were being dropped.
gdstk backend gaps
With the rename fixed, the opamp got further and hit four more gaps, found one
at a time:
Component.add()rejected a bareComponent. gdsfactory accepts one andreferences it implicitly; several composite cells rely on that.
diff_pair_stackedcmirrorimportedrectanglestraight from gdsfactory,bypassing the backend abstraction — the module already imports the backend's
rectangleat module level.ComponentReference.infoheld its own empty dict.mimcap_arraywritesinfo['netlist']on the Component whileopamp_twostagereads it back offthe reference returned by
<<; gdsfactory exposes the parent's info throughthe reference, so delegate.
Component.ref()took no arguments.opamp.py:155callsref(position=...).Results
opamp(gf180, ...), same parameters asGLayout_Cells.ipynbcell 14:No regression: under
gdsfactory,nmos(gf180, width=3, fingers=4)produces3328 ports and 875 polygons before and after.
Note
This makes the fast backend usable; it does not address why the slow one is
slow. Profiling the opamp under
gdsfactoryshows 96% of runtime insidepydantic validation, 6.3M
serialization.pycalls, and 1.85MPortobjectscreated — 93% of a transistor's ports belong to individual vias that are never
routed to. That is a port-model question, not something a compatibility fix
reaches.