-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: simpler LayerStack and DerivedLayers #2805
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.
gdsfactory/technology/layer_stack.py
Outdated
if isinstance(layer, GDSLayer): | ||
layer_index = get_layer(layer.layer) | ||
polygons = polygons_per_layer[layer_index] | ||
r = kf.kdb.Region(polygons) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Inline variable that is immediately returned [×2] (inline-immediately-returned-variable
)
I'm also removing all the clutter from as discussed in #2556 |
looks great, notice there are some conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
get_klayout_3d_script needs fixing, but it should be easy to convert the boolean operations in the layerstack to klayout script booleans |
there's issues with Pydantic to_json, will check again later |
…actory into 2556-clean-up-layerlevel
the pydantic serialization error is fixable by allowing layers to be int, is this acceptable behavior @joamatab @sebastian-goeldi ? |
gdsfactory/technology/layer_stack.py
Outdated
class LogicalLayer(BaseModel): | ||
"""GDS design layer.""" | ||
|
||
layer: tuple[int, int] | kf.LayerEnum | int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
gdsfactory/technology/layer_stack.py
Outdated
""" | ||
|
||
layer1: LogicalLayer | DerivedLayer | int | ||
layer2: LogicalLayer | DerivedLayer | int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
The problem with An int should be stricly used for layout purposes (it's even not so good to pass it through function calls tbh as they will be serialized as ints) To get tuple from LayerEnum: To get name: To get layerenum from tuple: Get LayerEnum by name: |
Great work @simbilod ! Just some ideas: |
when will this PR be ready to review? we plan to release gdsfactory8 in the coming days |
I've implemented @HelgeGehring 's suggestions, it still works exactly as I showed above @joamatab this is ready to review. I've added a NotImplementedError on the klayout 3d script for now. The only sketchy thing is the int type allowed for layer object to fix pydantic issues. Also the commit history is a bit messy, we can rebase against main if you want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have skipped reviewing this pull request. It looks like we've already reviewed this pull request.
Some export tests are still failing, probably just need to call |
Sounds good, will this work with the plugins? like gmsh? im having lots of issues with the gdsfactory8 brach in gplugins i think it would be great to use klayout instead of shapely there if possible |
The meshing should mostly be fine, it only relies on get_component or get_component_with_derived_layers, so if those work it will be OK. We might need to add a layer.layer here and there at most. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2805 +/- ##
==========================================
- Coverage 69.05% 68.63% -0.42%
==========================================
Files 296 296
Lines 15215 15235 +20
Branches 2225 2229 +4
==========================================
- Hits 10506 10457 -49
- Misses 4113 4183 +70
+ Partials 596 595 -1 ☔ View full report in Codecov by Sentry. |
I think for layers in general, I think we should just pass them as strings (i.e. layer name). The longer the more I think it's easier to do it that way. Getting them can be as easy as |
The current way to deal with "derived layers" (i.e. physical levels on the chip that do not map 1:1 to GDS design layers) is very convoluted. It requires defining etch and grow levels, with flags for what is etched into what, which borders on process emulation. This also does not generalize well to other common mapping between GDS layers and physical elements on the chip, for instance marker layers modifying shared process steps.
I propose instead, at the gdsfactory level, to only consider the final fabricated levels of the chip. These layers should be easily defined from arbitrary combinations of GDS layers. An example could look something like this:
Now get the component with derived layers according to the stack above:
(3,0) is the intersection:
(4,0) is the exclusive or:
(5,0) is a substraction:
(6,0) is union:
We can also compose the operations; (7,0) substracts the X0R from layer 1:
We can revisit process emulation and/or simulation in gplugins.
If you think this is a good idea I can make tests and fix visualization scripts