Fix lumerical extension #594
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideRefactored write_sparameters_lumerical to streamline margin handling, introduce exclude_layers filtering, unify layer tuple support and import logic, enforce component flattening/naming, and set cladding opacity; also updated get_meep_geometry to accept tuple layers. Sequence Diagram: Lumerical Geometry Setup in write_sparameters_lumericalsequenceDiagram
participant func as write_sparameters_lumerical
participant comp_ext as component_extended_beyond_pml
participant session as s (LumericalSession)
participant lstack as layer_stack
func->>comp_ext: flatten()
func->>comp_ext: name = "top"
func->>comp_ext: write_gds()
comp_ext-->>func: gdspath
func->>session: addfdtd(...)
func->>session: addmesh(...)
func->>session: addstructure("rect", name="clad")
func->>session: setnamed("clad", "alpha", 0.1)
func->>comp_ext: get_polygons_points(merge=True)
comp_ext-->>func: polygons_per_layer
func->>lstack: layers.values()
lstack-->>func: levels
loop For each level in levels
func->>func: Process level (get layer_tuple, layer_index)
alt if layer_index in exclude_layers OR not in polygons_per_layer
func->>func: Skip layer processing
else
func->>func: Get zmin, thickness, material details
func->>session: gdsimport(gdspath, "top", layer_tuple_string)
func->>session: setnamed(layername, "z", z_value)
func->>session: setnamed(layername, "z span", thickness_value)
func->>session: set_material(structure=layername, material=material_spec)
end
end
loop For each port
func->>func: Calculate port geometry (z, zspan) using updated index_to_zmin/thickness
func->>session: addport()
func->>session: Configure port properties (name, z, etc.)
end
Class Diagram: Updated signature for write_sparameters_lumerical functionclassDiagram
class write_sparameters_lumerical {
<<Function>>
+component: Component
+layer_stack: LayerStack
+dirpath: PathType
+port_source_name_function: Callable
+session: Optional[Any]
+layer_to_name: Optional[dict[Layer, str]]
+simulation_settings: SimulationSettings
+port_margin: float
+port_source_offset: float
+port_symmetries: Optional[list[str]]
+material_name_to_lumerical: Optional[dict[str, MaterialSpec]]
+delete_fsp_files: bool
+xmargin: float
+ymargin: float
+xmargin_left: Optional[float]
+xmargin_right: Optional[float]
+ymargin_top: Optional[float]
+ymargin_bot: Optional[float]
+zmargin: float
+exclude_layers: Optional[list[int]]
+settings: dict
+write_sparameters_lumerical(...): np.ndarray
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @joamatab - I've reviewed your changes - here's some feedback:
- Use explicit None checks for margin defaults (e.g.,
if xmargin_left is None:) rather than truthyorto allow zero margins. - Extract the repeated logic for deriving
layer_tuplefromlevelinto a helper to reduce duplication and simplify the index/thickness loops. - Replace
isinstance(..., tuple | LayerEnum)usages with a proper tuple of types (e.g.,(tuple, LayerEnum)) to avoid runtime TypeError.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+210
to
+213
| xmargin_left = xmargin_left or xmargin | ||
| xmargin_right = xmargin_right or xmargin | ||
| ymargin_top = ymargin_top or ymargin | ||
| ymargin_bot = ymargin_bot or ymargin |
There was a problem hiding this comment.
suggestion (bug_risk): Use explicit None check instead of or for margin defaults
Using or will override valid zero values. Use if xmargin_left is None: xmargin_left = xmargin to ensure only None triggers the default.
Suggested change
| xmargin_left = xmargin_left or xmargin | |
| xmargin_right = xmargin_right or xmargin | |
| ymargin_top = ymargin_top or ymargin | |
| ymargin_bot = ymargin_bot or ymargin | |
| if xmargin_left is None: | |
| xmargin_left = xmargin | |
| if xmargin_right is None: | |
| xmargin_right = xmargin | |
| if ymargin_top is None: | |
| ymargin_top = ymargin | |
| if ymargin_bot is None: | |
| ymargin_bot = ymargin |
| layer = level.layer | ||
|
|
||
| if isinstance(layer, LogicalLayer): | ||
| assert isinstance(layer.layer, tuple | LayerEnum) |
There was a problem hiding this comment.
issue (bug_risk): Invalid isinstance(..., tuple | LayerEnum) check
Use (tuple, LayerEnum) instead of tuple | LayerEnum in isinstance to avoid a TypeError.
This was referenced Jun 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes lumerical extension
Summary by Sourcery
Improve the Lumerical extension by enhancing margin controls, adding layer exclusion support, and standardizing layer handling for robust S-parameter simulations
New Features:
Enhancements: