gdsfactory / kfactory

gdsfactory with a klayout backend
https://gdsfactory.github.io/kfactory/
MIT License
34 stars 13 forks source link

Improve layers so that they can be passed easily by LayerInfo and not only by index #439

Closed sebastian-goeldi closed 3 months ago

sebastian-goeldi commented 4 months ago

TODO:

sourcery-ai[bot] commented 4 months ago

🧙 Sourcery has finished reviewing your pull request!


Tips - Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - Continue your discussion with Sourcery by replying directly to review comments. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.
sebastian-goeldi commented 4 months ago

The new structure will allow this:

import kfactory as kf

class Constants(kf.kcell.Constants):
    WG_WIDTH: int = 2000

class Layers(kf.kcell.LayerInfos):
    WG: kf.kdb.LayerInfo = kf.kdb.LayerInfo(1, 0, "WG")
    WGEX: kf.kdb.LayerInfo = kf.kdb.LayerInfo(1, 1, "WG")

kcl = kf.KCLayout[Layers, Constants]("TEST", layer_infos=Layers, constants=Constants)
c = kcl.kcell()

kcl.constants.WG_WIDTH

c.kcl.layer_infos.WG
c.kcl.layer_infos.CLAD

c.kcl.constants.WG
c.kcl.constatns.WGEX

Autocompletion currently only works for kcl.constants and kcl.layer_infos. image

Mypy is able to detect missing/wrong fields though image

Is the increased complexity worth it though? These changes will necessitate that we do

kcl.kcell() in the future instead of kf.KCell(...) (because otherwise we would need to do kf.KCell[MyLayerInfosClass, MyConstantsClass](kcl=kcl)

@gdspaul @joamatab @tvt173

gdspaul commented 4 months ago

do you mean that users who are happy to use the default kcl are still going to have to type kf.kcl.kcell to make a cell?

sebastian-goeldi commented 4 months ago

not really, this works perfectly fine (just mypy complaining in my case) and you don't get autocompletion for constants and layer_infos ofc:

import kfactory as kf

c = kf.KCell()

c.shapes(c.kcl.layer(1, 0)).insert(kf.kdb.Box(500))
c.show()
gdspaul commented 3 months ago

does the layerinfo init really need a layer name put in? it's been hard to sneak nice things past mypy here, but maybe there's a way to internally set layerinfo.name to match the layer attribute names without annoying mypy. pseudopython—

class Layers(baseclass):
 WG = layerinfo(1,0) # wire it up internally to become layerinfo(1,0,'WG')
sebastian-goeldi commented 3 months ago

does the layerinfo init really need a layer name put in? it's been hard to sneak nice things past mypy here, but maybe there's a way to internally set layerinfo.name to match the layer attribute names without annoying mypy. pseudopython—

class Layers(baseclass):
 WG = layerinfo(1,0) # wire it up internally to become layerinfo(1,0,'WG')

It does not in fact, but what it needs is the type annotation due to it being a pydantic model, unfortunately.

so it's happy with

class Layers(kf.kcell.LayerInfos):
    WG: kf.kdb.LayerInfo = kf.kdb.LayerInfo(1, 0)

and it will make it to WG: kf.kdb.LayerInfo = kf.kdb.LayerInfo(1, 0, "WG") but : kf.kdb.LayerInfo is necessary

gdspaul commented 3 months ago

pydantic sucks for user-facing classes anyway—what if you wrapped a simple Layers class in a pydantic model internally when it's passed to the KCLayout, so you can still serialize it (i assume that's how pydantic got involved here)? you can write something inside to do the annotations. then the layers class written by the user (pdk creator) is simpler.

i have found it handy to have a pydantic model inherit from a vanilla dataclass. i like hiding pydantic where nobody has to see it.

my suggestion would probably make autocomplete of pdk.layer.WG not work, but tbh I don't anticipate wanting to write pdk.layers.WG and get autocomplete there. it's too many dots for such a commonly-used piece of API real estate, esp. after I've got Layer.WG as my original source of truth.

sebastian-goeldi commented 3 months ago

dataclasses are also not great (i.e. they also need type hints) if you want stuff like asdict to work properly, so I don't have a great solution for that.

gdspaul commented 3 months ago

my comment was confusing, my bad. the layers concept isn't a good fit for dataclasses either. but maybe there's something that could be implemented—let's call it UserFacingLayersBaseClass, so user code could read

class Layers(UserFacingLayersBaseClass):
 WG = kf.kdb.LayerInfo(1,0)

which does not need to be annotated, and then there's some function in the KCLayout infrastructure that does this job:

# user_defined_layer_class = Layers
pdk_layers: ActualLayersPydanticModel = make_pydantic_thingy_from_user_class(user_defined_layer_class)
# validation is done
pdk.layers = pdk_layers
pdk.layers.model_dump() # but Layers.model_dump() is not defined
sebastian-goeldi commented 3 months ago

I don't think that would work. I mean you are free to do any of this. You just can't pass it to the KCLayout creation.

But you can always do this if you do not want autocomplete

kcl.layer_infos = kf.kcell.LayerInfos(WG=kf.kdb.LayerInfo(1,0))