fix(lumerical): extend ports beyond PML - #766
Open
joamatab wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideExtends physical port waveguides so they reach beyond the FDTD PML margins and adds a regression test to verify exported GDS bounds against FDTD simulation bounds, using a more stateful test session stub to capture FDTD and GDS settings. Sequence diagram for updated port extension beyond PML in write_sparameters_lumericalsequenceDiagram
participant write_sparameters_lumerical
participant extension_extend_ports
write_sparameters_lumerical->write_sparameters_lumerical: [compute port_extension_beyond_pml from ss.port_extension and xmargin_left/xmargin_right/ymargin_top/ymargin_bot]
write_sparameters_lumerical->extension_extend_ports: extend_ports(component_extended, port_extension_beyond_pml)
extension_extend_ports-->write_sparameters_lumerical: component_extended_beyond_pml
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: |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
port_extension_beyond_pml, usingmax(xmargin_left, xmargin_right, ymargin_top, ymargin_bot)assumes symmetric behavior across all orientations; if margins are intentionally asymmetric per side, consider computing the required extension per port direction instead of a global max to avoid over-extension on some ports. - In
test_port_extension_reaches_beyond_pml, relying on re-importing the written GDS path adds I/O and an extra dependency on the GDS import behavior; consider asserting against the bounds ofcomponent_extended_beyond_pml(or the object passed togdsimport) directly via a hook or helper to keep the test focused on the FDTD/port extension logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `port_extension_beyond_pml`, using `max(xmargin_left, xmargin_right, ymargin_top, ymargin_bot)` assumes symmetric behavior across all orientations; if margins are intentionally asymmetric per side, consider computing the required extension per port direction instead of a global max to avoid over-extension on some ports.
- In `test_port_extension_reaches_beyond_pml`, relying on re-importing the written GDS path adds I/O and an extra dependency on the GDS import behavior; consider asserting against the bounds of `component_extended_beyond_pml` (or the object passed to `gdsimport`) directly via a hook or helper to keep the test focused on the FDTD/port extension logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
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.pyuv run ruff format --check gplugins/lumerical/write_sparameters_lumerical.py gplugins/lumerical/tests/test_write_sparameters_lumerical.pySummary by Sourcery
Extend exported waveguides so that all ports reach beyond the FDTD PML margins and add coverage via regression testing.
Bug Fixes:
Tests: