Skip to content

better rings#209

Merged
joamatab merged 4 commits intomainfrom
better_rings
Apr 13, 2026
Merged

better rings#209
joamatab merged 4 commits intomainfrom
better_rings

Conversation

@joamatab
Copy link
Copy Markdown
Contributor

@joamatab joamatab commented Feb 21, 2026

  • better_rings
  • better_rings

Summary by Sourcery

Add port metadata to component regression fixtures, make ring_single bend type configurable, and improve GDS regression tooling and configuration.

Enhancements:

  • Expose configurable bend component and Euler fraction parameter for single-ring cells in both C-band and O-band libraries, delegating to gdsfactory's ring_single implementation.
  • Include ports in all component settings regression snapshots across PDK test suites and update YAML references accordingly.
  • Introduce a shared pytest difftest helper that produces XOR GDS diffs and optional rendered images, with an --update-gds-refs workflow.
  • Add sample scripts for debugging coupler cells in si500 and sin300 PDKs.
  • Extend Makefile with focused port-position tests and enhanced force-regeneration using updated GDS references.

Build:

  • Bump gdsfactory dependency from 9.34.0 to 9.35.1 in project configuration.

CI:

  • Add a dedicated test-ports Makefile target to run optical port position tests selectively.

Tests:

  • Update numerous si220, si500, and sin300 regression YAMLs to capture explicit port positions and types for optical and electrical components.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Feb 21, 2026

Reviewer's Guide

Adds 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 construction

sequenceDiagram
    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
Loading

File-Level Changes

Change Details Files
Make ring_single cells use a configurable bend component (with Euler fraction p) and delegate all geometry generation to gdsfactory’s canonical implementation.
  • Add bend and p parameters to ring_single in both si220 cband and oband libraries and document them.
  • Remove custom zero-length straight handling logic in ring_single and instead always call gf.c.ring_single.
  • Instantiate the bend argument by calling gf.get_component(bend, p=p, radius=radius, cross_section=cross_section) before passing it into gf.c.ring_single.
cspdk/si220/cband/cells/rings.py
cspdk/si220/oband/cells/rings.py
Extend settings regression tests to include port data and regenerate all YAML fixtures with explicit ports.
  • Change test_settings in all process-specific test modules to call component.to_dict(with_ports=True).
  • Update all existing settings.yml regression files to include a ports section for each component or an explicit empty ports: {} when appropriate.
  • Add a dedicated Make target test-ports to run optical port position tests across multiple PDKs.
tests/test_si220_cband.py
tests/test_si220_oband.py
tests/test_si500.py
tests/test_sin300.py
tests/test_si220_cband/*.yml
tests/test_si220_oband/*.yml
Makefile
Introduce a custom difftest helper that produces GDS XOR files and optional PNG renders, plus a workflow to update reference GDS files via a pytest flag.
  • Add a shared tests/conftest.py that defines a pytest --update-gds-refs option and a global flag.
  • Implement a difftest() helper that writes run-time GDS, compares with reference, generates XOR diff GDS via gdsfactory.difftest.diff, and optionally renders diff images using klayout’s headless LayoutView.
  • When --update-gds-refs is set, open the diff GDS in KLayout and interactively ask whether to overwrite the reference; otherwise fail with paths and guidance on how to update refs.
tests/conftest.py
Update tooling and dependencies for GDS regression management and debugging.
  • Change test-force Make target to run pytest with --update-gds-refs along with --force-regen for si220 cband/oband tests (and commented-out others).
  • Bump gdsfactory dependency from ~9.34.0 to ~9.35.1 to align with the new functionality and data formats.
  • Add small sample scripts for si500 and sin300 PDKs to quickly visualize specific coupler-like cells that may have port issues.
Makefile
pyproject.toml
cspdk/si500/samples/all_cells2.py
cspdk/sin300/samples/all_cells2.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +9 to +11
cell_name = "coupler"
cell_name = "coupler_rc"
cell_name = "coupler_ro"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@ThomasPluck ThomasPluck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix tests

@joamatab joamatab merged commit 58e349c into main Apr 13, 2026
5 of 6 checks passed
@joamatab joamatab deleted the better_rings branch April 13, 2026 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants