Conversation
Reviewer's GuideThis PR updates the Lumerical s-parameters writer to use the Layer Builder for layers with nonzero sidewall angles while keeping the existing GDS import path for vertical sidewalls, and adds tests to validate the sidewall angle mapping and Layer Builder behavior. Sequence diagram for Layer Builder vs GDS import in write_sparameters_lumericalsequenceDiagram
participant write_sparameters_lumerical
participant LayerStack
participant LumericalSession_s
write_sparameters_lumerical->>LayerStack: iterate layers
loop for each level in layer_stack.layers.values
write_sparameters_lumerical->>LayerStack: get level.sidewall_angle
alt level.sidewall_angle is nonzero
write_sparameters_lumerical->>LumericalSession_s: addlayerbuilder()
write_sparameters_lumerical->>LumericalSession_s: set(name = gplugins_layer_builder)
write_sparameters_lumerical->>LumericalSession_s: loadgdsfile(gdspath)
write_sparameters_lumerical->>LumericalSession_s: addlayer(layer_layer_tuple)
write_sparameters_lumerical->>LumericalSession_s: setlayer(layer_layer_tuple, "layer number", layer_tuple)
write_sparameters_lumerical->>LumericalSession_s: setlayer(layer_layer_tuple, "start position", zmin)
write_sparameters_lumerical->>LumericalSession_s: setlayer(layer_layer_tuple, "thickness", thickness)
write_sparameters_lumerical->>LumericalSession_s: setlayer(layer_layer_tuple, "sidewall angle", 90 - level.sidewall_angle)
write_sparameters_lumerical->>LumericalSession_s: setlayer(layer_layer_tuple, "pattern material", material)
else level.sidewall_angle is zero
write_sparameters_lumerical->>LumericalSession_s: gdsimport(gdspath, "top", layer_tuple)
write_sparameters_lumerical->>LumericalSession_s: setnamed(GDS_LAYER_layer_tuple, "z", z)
write_sparameters_lumerical->>LumericalSession_s: setnamed(GDS_LAYER_layer_tuple, "z span", thickness)
write_sparameters_lumerical->>LumericalSession_s: set_material(session = s, structure = GDS_LAYER_layer_tuple, material = material)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Label error. Requires at least 1 of: breaking, bug, github_actions, documentation, dependencies, enhancement, feature, maintenance, security. Found: |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
if level.sidewall_angle:check will skip using the layer builder whensidewall_angleis 0 but also if it isNone; consider explicitly distinguishing betweenNoneand numeric values to avoid surprises ifsidewall_angle=0.0is ever meaningful. - The material type validation for sidewall-angle layers is done inside the loop; if non-string materials are allowed elsewhere, it might be clearer to validate or convert materials before this branch so the behaviour is consistent across layer types.
- The
layer_builder_nameconstant is set but only used in a singlesetcall; consider inlining it or moving it to a shared constant if it needs to be reused/configured in other places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `if level.sidewall_angle:` check will skip using the layer builder when `sidewall_angle` is 0 but also if it is `None`; consider explicitly distinguishing between `None` and numeric values to avoid surprises if `sidewall_angle=0.0` is ever meaningful.
- The material type validation for sidewall-angle layers is done inside the loop; if non-string materials are allowed elsewhere, it might be clearer to validate or convert materials before this branch so the behaviour is consistent across layer types.
- The `layer_builder_name` constant is set but only used in a single `set` call; consider inlining it or moving it to a shared constant if it needs to be reused/configured in other places.
## Individual Comments
### Comment 1
<location path="gplugins/lumerical/write_sparameters_lumerical.py" line_range="521-526" />
<code_context>
+ s.setlayer(layername, "start position", zmin * 1e-6)
+ s.setlayer(layername, "thickness", thickness * 1e-6)
+ s.setlayer(layername, "sidewall angle", 90 - level.sidewall_angle)
+ if not isinstance(material, str):
+ raise ValueError(
+ "Layer Builder sidewall angles require a material database name. "
+ f"Got {material!r} for layer {layer_tuple}."
+ )
+ s.setlayer(layername, "pattern material", material)
+ else:
+ s.gdsimport(str(gdspath), "top", f"{layer_tuple[0]}:{layer_tuple[1]}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Material handling diverges between sidewall and non-sidewall paths and may break non-string material usages.
In the previous flow, `set_material` accepted various material representations for all layers. In the sidewall-angle branch you now enforce `material` to be a string and skip `set_material`, which will raise for existing callers that pass non-string materials that were previously valid. It also introduces inconsistent type requirements between sidewall and non-sidewall paths. Please either normalize `material` to a string earlier and continue using `set_material`, or otherwise ensure both paths accept the same material types and follow a single configuration mechanism.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if not isinstance(material, str): | ||
| raise ValueError( | ||
| "Layer Builder sidewall angles require a material database name. " | ||
| f"Got {material!r} for layer {layer_tuple}." | ||
| ) | ||
| s.setlayer(layername, "pattern material", material) |
There was a problem hiding this comment.
issue (bug_risk): Material handling diverges between sidewall and non-sidewall paths and may break non-string material usages.
In the previous flow, set_material accepted various material representations for all layers. In the sidewall-angle branch you now enforce material to be a string and skip set_material, which will raise for existing callers that pass non-string materials that were previously valid. It also introduces inconsistent type requirements between sidewall and non-sidewall paths. Please either normalize material to a string earlier and continue using set_material, or otherwise ensure both paths accept the same material types and follow a single configuration mechanism.
Summary
Validation
uv run pytest gplugins/lumerical/tests/test_write_sparameters_lumerical.py gplugins/lumerical/tests/test_background_layers.py gplugins/lumerical/tests/test_netlist.py gplugins/lumerical/tests/test_netlist_get_routes.py -quv run ruff check gplugins/lumerical/write_sparameters_lumerical.py gplugins/lumerical/tests/test_write_sparameters_lumerical.pySummary by Sourcery
Support sidewall angles in Lumerical exports by using Layer Builder for non-vertical LayerStack layers while retaining GDS import for vertical layers.
New Features:
Enhancements:
Tests: