Skip to content

fix(palace): align PDK stack defaults#151

Open
mdmaas wants to merge 3 commits into
gdsfactory:mainfrom
EpsilonForge:fix/pdk-stack-no-autocap
Open

fix(palace): align PDK stack defaults#151
mdmaas wants to merge 3 commits into
gdsfactory:mainfrom
EpsilonForge:fix/pdk-stack-no-autocap

Conversation

@mdmaas
Copy link
Copy Markdown
Contributor

@mdmaas mdmaas commented May 29, 2026

This PR aligns Palace stack/mesh behavior with strict PDK-defined defaults: include what the PDK defines by default.

In the electrostatic notebook, it also improves the analytical capacitor estimate to read geometry and stack values directly instead of relying on hardcoded constants.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the Palace simulation stack extraction and geometry configuration by introducing top_layer and max_z options to cap dielectric/air construction, dynamically resolving bulk dielectric materials from PDK definitions, and preferring explicit PDK passivation layers over synthetic ones. Additionally, notebooks and default configurations (such as setting default air_above and margin to zero) are updated, alongside comprehensive unit tests. The review feedback correctly identifies a missing import of SimpleNamespace in tests/palace/test_sim_classes.py that would cause a runtime error, and suggests a valuable validation check to prevent non-physical oxide thicknesses when oxide_zmax is less than or equal to oxide_zmin.

Comment thread tests/palace/test_sim_classes.py Outdated
Comment on lines +7 to +10
import pytest

from gsim.common import Geometry
from gsim.common.stack import Layer, LayerStack
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The test test_resolve_stack_does_not_auto_cap_from_geometry uses SimpleNamespace on line 204, but it is not imported in this file. This will cause a NameError when the test is executed. Please import SimpleNamespace from types.

Suggested change
import pytest
from gsim.common import Geometry
from gsim.common.stack import Layer, LayerStack
import pytest
from types import SimpleNamespace
from gsim.common import Geometry
from gsim.common.stack import Layer, LayerStack

Comment on lines +537 to +539
oxide_zmax = (
z_max_active if passive_zmin is None else min(passive_zmin, z_max_active)
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If oxide_zmax is calculated to be less than or equal to oxide_zmin (for example, if a passivation layer or z-cap is placed below the oxide starting height), the oxide layer will have a non-physical zero or negative thickness. We should add a check to raise a ValueError in this case to prevent silent failures or crashes during meshing/simulation.

    oxide_zmax = (
        z_max_active if passive_zmin is None else min(passive_zmin, z_max_active)
    )
    if oxide_zmax <= oxide_zmin:
        raise ValueError(
            f"Non-physical oxide thickness: zmin={oxide_zmin} um, zmax={oxide_zmax} um. "
            "Ensure passivation layers or z-caps are above the oxide starting height."
        )

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.

1 participant