Fix fibre port placed off the fibre axis in get_simulation_grating_fiber - #776
Open
Alisama20 wants to merge 1 commit into
Open
Fix fibre port placed off the fibre axis in get_simulation_grating_fiber#776Alisama20 wants to merge 1 commit into
Alisama20 wants to merge 1 commit into
Conversation
…_fiber fiber_port_x_offset_from_angle was computed from a fiber_port_y that omitted pml_thickness, substrate_thickness and box_thickness. A few lines later fiber_port_y was reassigned to the full sum and used for the port centre, but the x offset was never recomputed, so the fibre port was placed at the x of a height it is not at. With the library defaults that puts the port 0.746 um off the fibre core, measured from the simulation's own epsilon along the port line: port centred at x=0.3239 um, core centred at x=1.0701 um, while the fibre axis at that height is at x=1.0592 um. The error is y_error * tan(fiber_angle), so it grows with the fibre angle. It also feeds sxy, making the cell narrower than the requested margin. Fixed by computing fiber_port_y once, correctly, and deleting the duplicate. After the fix the port lands 0.0728 um from the core centre, which is exactly the monitor's deliberate 0.2 um y standoff projected on the tilted axis (0.2 * tan(20 deg) = 0.0728). Adds the first test for this module. It locates the fibre core by probing the simulation's own dielectric along the port line, so it does not read any port variable and cannot agree with a mis-placed port by construction. Verified to fail on the unfixed code (off by -0.7462 um against a 0.2061 um tolerance) at both 10 and 20 degrees, and to pass after.
Alisama20
requested review from
ThomasPluck,
joamatab and
nikosavola
as code owners
September 6, 2026 09:05
Contributor
Reviewer's GuideFixes fibre-port misalignment by calculating its vertical position once from the full layer stack before deriving the angle-dependent x offset, and adds independent epsilon-based regression coverage at two fibre angles. Sequence diagram for fibre port axis regression testsequenceDiagram
participant Test
participant Builder as get_simulation_grating_fiber
participant Epsilon as Built dielectric
Test->>Builder: get_simulation_grating_fiber(fiber_angle_deg)
Builder-->>Test: Simulation with fibre port
Test->>Epsilon: Probe dielectric along port line
Epsilon-->>Test: Fibre core edge positions
Test->>Test: Compare core centre with port centre
alt within derived tolerance
Test-->>Test: Pass at 10 and 20 degrees
else outside tolerance
Test-->>Test: Fail for stale port offset
end
Flow diagram for corrected fibre port placementflowchart TD
A[Build full layer stack] --> B[Compute fiber_port_y once]
B --> C[Compute fiber_port_x_offset_from_angle]
C --> D[Create fiber_port_center]
D --> E[Fibre port follows tilted fibre axis]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Sep 6, 2026
Contributor
Author
|
@joamatab could you add a label here when you get a chance? Everything else is green, including |
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.
What
get_simulation_grating_fiberplaces the fibre port off the fibre.fiber_port_x_offset_from_angleis computed from afiber_port_ythat omitspml_thickness,substrate_thicknessandbox_thickness. Forty lines laterfiber_port_yis reassigned to the full sum and used forfiber_port_center,but the x offset is never recomputed — so the port's x is the x of a height the
port is not at.
The fibre core block is built centred on
x=0and rotated byfiber_angle, soits axis crosses height
yatx = y*tan(fiber_angle). Gettingywrongdisplaces the port along x by
y_error * tan(fiber_angle), growing with thefibre angle.
Evidence
Measured from the simulation's own epsilon, sampled along the fibre port line
(defaults,
fiber_angle_deg=20):The measured core centre and the axis prediction agree to 0.011 um, which is
what confirms the port — not the geometry — is the thing that is misplaced.
The stale offset also feeds
sxy, so the cell comes out narrower than thexmarginthat was asked for.Fix
Compute
fiber_port_yonce, correctly, and delete the duplicate. Net effect isthree fewer lines of logic and one definition instead of two.
After the fix the port lands 0.0728 um from the core centre. That residue is
not error: it is the monitor's deliberate 0.2 um standoff in y, projected onto
the tilted axis,
0.2 * tan(20 deg) = 0.0728— agreeing to four decimals.Test
This adds the first test for this module. It finds the fibre core by probing
the built dielectric along the port line, so it reads no port variable and
cannot agree with a misplaced port by construction. The tolerance is derived,
not tuned: the monitor's y standoff projected on the axis, plus two pixels for
the resolution of the core edges.
Verified to fail on the unfixed code and pass on the fixed one, at two angles:
Run in the meep environment (meep 1.34.0, gdsfactory 9.34.2).
Scope
This does not touch the S-parameter extraction. It came up while investigating
the fibre port for a separate issue about
s22, which I am filing separately.Summary by Sourcery
Align the grating-fibre port with the fibre axis and add regression coverage for angled fibre configurations.
Bug Fixes:
Tests: