Repository navigation
Conversation
grid_with_text placed each label with t.move() and only then applied text_mirror and text_rotation. Both rotate() and dmirror() act about the layout origin, so applying them after the move dragged the label away from the component it names: with text_rotation=180 the labels ended up reflected through (0, 0), tens of microns off the grid. gdsfactory/grid.py now mirrors and rotates the label before moving it to the anchor, which is the order pack() and edge_coupler_array() already use. Every current caller uses the defaults text_rotation=0 and text_mirror=False, so no reference GDS or netlist changes.
MaxFreedomPollard
requested review from
ThomasPluck,
flaport,
joamatab,
nikosavola,
sebastian-goeldi and
thanojo
as code owners
September 6, 2026 06:55
Contributor
Reviewer's GuideFixes grid_with_text label placement by applying mirroring and rotation before moving labels to their anchors, with regression tests covering all supported rotations and mirroring. Sequence diagram for corrected grid label placementsequenceDiagram
participant Grid as grid_with_text
participant Label as LabelInstance
participant Anchor as CellAnchor
Grid->>Label: dmirror()
Grid->>Label: rotate(text_rotation)
Grid->>Anchor: dsize_info[text_anchor]
Grid->>Label: move(text_offset + anchor)
Anchor-->>Label: Label remains on anchor
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}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4821 +/- ##
=======================================
Coverage 76.20% 76.21%
=======================================
Files 330 330
Lines 21696 21696
Branches 2810 2810
=======================================
+ Hits 16533 16535 +2
Misses 4140 4140
+ Partials 1023 1021 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
grid_with_textmoved each label to its anchor first and appliedtext_mirrorandtext_rotationafterwards.rotate()anddmirror()act about the layout origin, not about the instance, so doing them last dragged every label away from the cell it names. On a 3 cell grid of 1x1 rectangles withtext_rotation=180, the labels land at (0, -3), (-6, -3) and (-12, -3) instead of on their anchors at (0, 3), (6, 3) and (12, 3).text_mirror=Truescatters them along negative x, and the offset grows with the grid, so on a real mask the labels end up nowhere near the devices.The fix in gdsfactory/grid.py:171-182 mirrors and rotates the label before moving it to the anchor. That is the order
packalready uses (gdsfactory/pack.py:273-280) and the orderedge_coupler_arrayuses (gdsfactory/components/edge_couplers/edge_coupler_array.py:88-91), so this makesgrid_with_textagree with the rest of the library rather than inventing a new convention.Every caller in the repo,
pack_doe_gridincluded, uses the defaultstext_rotation=0andtext_mirror=False, where both branches are skipped. No reference GDS, netlist or settings fixture changes.Test Plan
Added
test_grid_with_text_rotation_keeps_labels_on_anchors(parametrized over 0, 90, 180, 270) andtest_grid_with_text_mirror_keeps_labels_on_anchorsto tests/test_grid.py. Each asserts that every label instance sits at thecenteranchor of the cell it labels.uv run pytest tests/test_grid.pyon main with only the grid.py change reverted: 4 failed, 3 passed. The 90, 180 and 270 rotations and the mirror case fail,text_rotation=0passes. With the fix: 7 passed.uv run pytest -q -n 2 tests/aftermake test-data: 1731 passed, 2 skipped, 1 xfailed.uv run pytest -q tests/components/test_components.py -k "pack_doe or grid": 6 passed, so thepack_doe_gridandbump_pad_gridgeometry still matches the gdsfactory-test-data references.uv run pre-commit run --files gdsfactory/grid.py tests/test_grid.py: all hooks pass, including ruff check, ruff format, codespell and pyright.Summary by Sourcery
Fix grid_with_text label placement by applying text transformations before anchoring labels to grid cells.
Bug Fixes:
Tests: