Skip to content

mimcap: build with extensions again - #113

Merged
msaligane merged 2 commits into
ReaLLMASIC:mainfrom
carloscl03:mimcap-extension-fix
Aug 27, 2026
Merged

mimcap: build with extensions again#113
msaligane merged 2 commits into
ReaLLMASIC:mainfrom
carloscl03:mimcap-extension-fix

Conversation

@carloscl03

Copy link
Copy Markdown
Contributor

mimcap() raises with its default arguments, on both options:

File "src/glayout/primitives/mimcap.py", line 293, in mimcap
    appendix_pos[cord_ref] = appendix_pos[cord_ref] + appendix_size[cord_ref]/2 ...
TypeError: 'tuple' object does not support item assignment

Port.center is a tuple and the line below indexes into it to shift the
appendix. Take a list.

The branch is only reached when min_via_distance < 0.4, so whether it fires
depends on the requested size — 5x5 hits it. No CI cell caught this because
every tested cell goes through mimcap_array, which passes
with_extension=False and skips the path entirely. A direct
mimcap(pdk, size) — the obvious way to use the primitive — fails.

Verified on gf180 with the gdstk backend:

call before after
mimcap(pdk, (5,5), option="A") TypeError builds
mimcap(pdk, (5,5), option="B") TypeError builds
mimcap(pdk, (10,4), option="B") TypeError builds
mimcap_array(pdk, 2, 2, (5,5)) builds builds

Layers written are as expected for each option — A: met2 / FuseTop / via2 /
met3; B: met4 / FuseTop / via4 / met5.

`mimcap()` raises with its default arguments, on both options:

    File "src/glayout/primitives/mimcap.py", line 293, in mimcap
        appendix_pos[cord_ref] = appendix_pos[cord_ref] + ...
    TypeError: 'tuple' object does not support item assignment

`Port.center` is a tuple, and the line below indexes into it to shift the
appendix. Take a list.

The branch is only reached when `min_via_distance < 0.4`, so it depends on
the requested size -- 5x5 hits it. No CI cell caught this because every
tested cell goes through `mimcap_array`, which passes `with_extension=False`
and skips the path. A direct `mimcap(pdk, size)` -- the obvious way to use
the primitive -- fails.

Verified on gf180: option A, option B, a non-square 10x4, and mimcap_array
all build and write GDS. Option A draws met2/FuseTop/via2/met3, option B
met4/FuseTop/via4/met5.
@LuighiV

LuighiV commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Hi @carloscl03

Thanks for catching this up. It is a bit strange as I remember tested the simple version of mimcap and it built without raising any issue.

Let me check again.

Can you please, add as part of the PR, an additional check for the individual mimcap?

Thanks,
Luighi

Every cell in the suite reaches mimcap through mimcap_array, which passes
`with_extension=False` and skips the branch that regressed. So the suite
stayed green while a direct `mimcap(pdk, size)` raised -- and anyone
checking the primitive by hand through the array saw it build fine.

Four cases: the default path with extensions, the array path, the
no-extension path kept so a fix that trades one branch for the other does
not read as a pass, and a check that each option draws the stack it names
(A on met2/met3, B on met4/met5) rather than merely building.

Against `main` this fails 6 of them with the TypeError; with the fix, 4
tests and 8 subtests pass.
@carloscl03

Copy link
Copy Markdown
Contributor Author

Thanks Luighi — added tests/test_mimcap_build.py.

Your recollection is right, and it is exactly why this went unnoticed. mimcap_array calls the primitive with with_extension=False, so the array path never reaches the appendix code. Anyone checking the primitive through the array — which is how every cell in the suite uses it — sees it build cleanly. It is the direct mimcap(pdk, size) that goes down the other branch and raises. On main today:

mimcap(pdk, size=(5.0, 5.0))                        TypeError
mimcap(pdk, size=(5.0, 5.0), with_extension=False)  builds, 0 DRC violations
mimcap_array(pdk, rows=2, columns=2, size=(5.0,5.0)) builds, 0 DRC violations

There is a second condition worth knowing: the appendix branch is only entered when the requested size leaves min_via_distance < 0.4, so it also depends on the size. 5x5 reaches it, which is why the default arguments fail.

The test covers four things:

  • the default path, with extensions, on both options — the one that regressed;
  • the array path;
  • the no-extension path, kept deliberately so a future fix that repairs one branch by breaking the other does not read as a pass;
  • that each option draws the stack it names — A on met2/met3, B on met4/met5 — read back from the written GDS so it does not depend on which backend is active. Building both options is not enough on its own, since they differ only in the metals they draw.

Verified it discriminates: against main it fails 6, with the fix 4 tests and 8 subtests pass.

@carloscl03

Copy link
Copy Markdown
Contributor Author

One more data point on this, from measuring the notebook CI on the fork.

The bug here is not confined to a direct mimcap() call — it breaks a tutorial. Running the suite on the gdstk backend, GLayout_Cells raises at mimcap.py:293 with exactly this TypeError. With this PR applied it builds and passes in 14.2s.

That matters because the notebooks job is currently the reason six of the eight open PRs show red: GLayout_Cells and glayout_opamp sit on the 180s per-cell limit and cross it depending on which runner the job lands, so the check flips on identical content. Moving the job to gdstk takes the whole suite from 660s to 158s and off the limit entirely — but only with this PR in, otherwise GLayout_Cells raises here instead of timing out.

Measured on this fork's CI, same container and runners: gdsfactory baseline · gdstk. Full table in #116.

The test added earlier still discriminates: 6 failures against main, 4 tests and 8 subtests passing with the fix.

msaligane pushed a commit that referenced this pull request Aug 25, 2026
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.
@msaligane
msaligane merged commit 3e87142 into ReaLLMASIC:main Aug 27, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants