Conversation
Reviewer's GuideAdds configurable bend components to ring_single cells, updates regression tests to capture explicit port information and support visual GDS XOR diffs with an optional reference-update workflow, bumps gdsfactory, and introduces small utilities and Make targets for debugging and port-position testing. Sequence diagram for configurable bend in ring_single constructionsequenceDiagram
actor User
participant ring_single_si220_cband as ring_single_si220_cband
participant ring_single_si220_oband as ring_single_si220_oband
participant gdsfactory as gf
participant gdsfactory_components as gf_c
User->>ring_single_si220_cband: ring_single(gap, radius, length_x, length_y, cross_section, length_extension, bend, p)
ring_single_si220_cband->>gdsfactory: get_component(bend, p, radius, cross_section)
gdsfactory-->>ring_single_si220_cband: bend_component
ring_single_si220_cband->>gdsfactory_components: ring_single(gap, radius, length_x, length_y, bend_component, straight, coupler_ring, cross_section, length_extension)
gdsfactory_components-->>ring_single_si220_cband: ring_device
ring_single_si220_cband-->>User: ring_device
User->>ring_single_si220_oband: ring_single(gap, radius, length_x, length_y, cross_section, bend, p)
ring_single_si220_oband->>gdsfactory: get_component(bend, p, radius, cross_section)
gdsfactory-->>ring_single_si220_oband: bend_component
ring_single_si220_oband->>gdsfactory_components: ring_single(gap, radius, length_x, length_y, bend_component, straight, coupler_ring, cross_section)
gdsfactory_components-->>ring_single_si220_oband: ring_device
ring_single_si220_oband-->>User: ring_device
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
difftesthelper intests/conftest.pycontradicts its own docstring ("without prompting") by callinginput()when--update-gds-refsis set; consider either removing the interactive prompt or updating the description/flag semantics so the behavior is clear and non-interactive by default for CI. - The custom GDS diff logic in
tests/conftest.pydepends onklayout.lay.LayoutView, but klayout is not listed as an explicit dependency; consider guarding these imports/usages or adding klayout as a test/dev dependency to avoid import errors in environments without it. - In the
ring_singlewrappers, the previous special handling forlength_x == 0orlength_y == 0has been removed; if those zero-length configurations are still expected to be supported, you may want to confirm thatgf.c.ring_singlenow handles them correctly or reintroduce a minimal guard to avoid port-overlap issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `difftest` helper in `tests/conftest.py` contradicts its own docstring ("without prompting") by calling `input()` when `--update-gds-refs` is set; consider either removing the interactive prompt or updating the description/flag semantics so the behavior is clear and non-interactive by default for CI.
- The custom GDS diff logic in `tests/conftest.py` depends on `klayout.lay.LayoutView`, but klayout is not listed as an explicit dependency; consider guarding these imports/usages or adding klayout as a test/dev dependency to avoid import errors in environments without it.
- In the `ring_single` wrappers, the previous special handling for `length_x == 0` or `length_y == 0` has been removed; if those zero-length configurations are still expected to be supported, you may want to confirm that `gf.c.ring_single` now handles them correctly or reintroduce a minimal guard to avoid port-overlap issues.
## Individual Comments
### Comment 1
<location> `cspdk/si500/samples/all_cells2.py:9-11` </location>
<code_context>
+
+if __name__ == "__main__":
+ PDK.activate()
+ cell_name = "coupler"
+ cell_name = "coupler_rc"
+ cell_name = "coupler_ro"
+ c = gf.get_component(cell_name)
+ c.show()
</code_context>
<issue_to_address>
**nitpick:** Repeated reassignment of `cell_name` makes it unclear which sample is intended and makes it harder to switch between options.
Only the final assignment (`"coupler_ro"`) actually takes effect; the earlier ones are dead code. To make this helper clearer, either keep only the active assignment and comment out alternatives, or add a small loop/CLI arg to iterate over different `cell_name` values when you want to visualize multiple cells.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| cell_name = "coupler" | ||
| cell_name = "coupler_rc" | ||
| cell_name = "coupler_ro" |
There was a problem hiding this comment.
nitpick: Repeated reassignment of cell_name makes it unclear which sample is intended and makes it harder to switch between options.
Only the final assignment ("coupler_ro") actually takes effect; the earlier ones are dead code. To make this helper clearer, either keep only the active assignment and comment out alternatives, or add a small loop/CLI arg to iterate over different cell_name values when you want to visualize multiple cells.
Summary by Sourcery
Add port metadata to component regression fixtures, make ring_single bend type configurable, and improve GDS regression tooling and configuration.
Enhancements:
Build:
CI:
Tests: