Fix s22 silently copied from s11 in write_sparameters_grating - #772
Conversation
s22 was a copy of s11 (the waveguide port's reflection coefficient), so o2@0,o2@0 returned the wrong physical quantity - the waveguide-side reflection instead of the fibre-side one. b2 (the fibre mode's backward wave) was already extracted one line above and simply unused. Fixes gdsfactory#755
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCorrects the grating S-parameter calculation so s22 uses the fibre mode’s measured backward-to-forward wave ratio, eliminating the physically invalid s22=s11 copy without adding a new simulation monitor. Flow diagram for corrected grating S-parameter calculationflowchart LR
FiberMode[fiber_mode alpha coefficients] --> A2[a2 forward wave]
FiberMode --> B2[b2 backward wave]
A2 --> S22[s22 = squeeze b2 / a2]
B2 --> S22
S11[s11 = squeeze b1 / a1] -. no longer copied to .-> S22
File-Level Changes
Assessment against linked issues
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}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="gplugins/gmeep/write_sparameters_grating.py" line_range="218-222" />
<code_context>
a2 = fiber_mode.alpha[:, :, idx].flatten() # forward wave
- # b2 = fiber_mode.alpha[:, :, 1 - idx].flatten() # backward wave
+ b2 = fiber_mode.alpha[:, :, 1 - idx].flatten() # backward wave
s11 = np.squeeze(b1 / a1)
s12 = np.squeeze(a2 / a1)
- s22 = s11.copy()
+ s22 = np.squeeze(b2 / a2)
s21 = s12.copy()
</code_context>
<issue_to_address>
**issue (broader_impact):** `s22 = np.squeeze(b2 / a2)` does not compute the fibre-port reflection coefficient. This simulation excites only the waveguide, so `a2` is the fibre-side wave generated by the waveguide excitation and `b2` is the opposite-direction coefficient under that same excitation; their ratio is not the response to an independent fibre-side incident wave and therefore does not equal S22. It will generally be zero or otherwise contaminated by numerical/radiative fields, and it can also produce NaN or infinity where `a2` is zero.
**Triggers:** When the grating is simulated with the existing waveguide-only source, especially near wavelengths with weak fibre coupling.
**Suggested fix:** Run a second simulation with the fibre mode excited and compute the fibre reflection as the outgoing fibre coefficient divided by the independently injected fibre coefficient; do not derive S22 from `b2 / a2` in the waveguide-excited run.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: gplugins/gmeep/write_sparameters_grating.py:222
| b2 = fiber_mode.alpha[:, :, 1 - idx].flatten() # backward wave | ||
|
|
||
| s11 = np.squeeze(b1 / a1) | ||
| s12 = np.squeeze(a2 / a1) | ||
| s22 = s11.copy() | ||
| s22 = np.squeeze(b2 / a2) |
There was a problem hiding this comment.
issue (broader_impact): s22 = np.squeeze(b2 / a2) does not compute the fibre-port reflection coefficient. This simulation excites only the waveguide, so a2 is the fibre-side wave generated by the waveguide excitation and b2 is the opposite-direction coefficient under that same excitation; their ratio is not the response to an independent fibre-side incident wave and therefore does not equal S22. It will generally be zero or otherwise contaminated by numerical/radiative fields, and it can also produce NaN or infinity where a2 is zero.
Triggers: When the grating is simulated with the existing waveguide-only source, especially near wavelengths with weak fibre coupling.
Suggested fix: Run a second simulation with the fibre mode excited and compute the fibre reflection as the outgoing fibre coefficient divided by the independently injected fibre coefficient; do not derive S22 from b2 / a2 in the waveguide-excited run.
|
Verified end to end against a real (small/fast) grating+fibre simulation Before the fix: `s22` identical to `s11` (max diff = 0.0) - reproduces After the fix: `s11 = [-0.0508+0.074j, -0.109+0.048j, -0.067-0.076j]`, |
|
Following up on my own verification comment above, because it claimed more Sourcery's blocking finding on this PR said that I have now run one.
Across five structures and three wavelengths, So the value now returned for I tried to implement Sourcery's suggested fix (a second, fibre-excited run) Separately, while looking at the fibre port for this I found that it is placed |
2 participants
What's broken
In
write_sparameters_grating.py, only the waveguide port is ever excited,so
s11(reflection back into the waveguide) ands12(waveguide-to-fibrecoupling) are genuinely measured:
s21 = s12.copy()is at least defensible by Lorentz reciprocity, buts22 = s11.copy()has no such justification.s22is supposed to be thefibre port's own reflection coefficient when excited from the fibre side -
a physically different quantity, on a different mode, in a different
radiation environment. The variable needed to compute it correctly (
b2,the fibre mode's backward wave) is extracted one line above and simply
never used.
The fix
No second Meep monitor is needed -
fiber_modealready carries thebackward coefficient, it just wasn't read.
Testing
This is a one-line-substitution fix using data already computed by the
existing simulation, not a new measurement path - the risk surface is
small. Happy to add a regression test asserting
s22 != s11on anasymmetric grating if useful.
Fixes #755
Summary by Sourcery
Bug Fixes:
s22from the measured backward fibre wave instead of copying the waveguide reflection coefficients11.