Repository navigation
fix(femwell): solve modes at element order 2 by default - #483
Conversation
At element order 1, the series impedance of a lossy metal line read from a femwell mode (gamma * Z0) comes out several percent low on a mesh that is fine at the conductors and coarse in the dielectric. Refining only near the conductors does not fix it; order 2 on the same mesh does. solve_modes now defaults to order=2, so RF line solves, including the TW-MZM notebook's, get the accurate series side without extra arguments. gsim.femwell first appeared after 0.8.0 (gdsfactory#298) and has not been released, so the default changes without a deprecation period. The notebook's optical solve now passes order=1, so its per-bias cost and results stay as they were. Two tests that do not depend on the element order also pin order=1: the staircase convergence test, whose five solves on one mesh got about seven times slower, and the Palace cross-validation test, whose tolerance was set for order 1. A regression test solves an aluminium strip in a PEC box at 1 GHz and checks that its series resistance matches the strip's DC resistance to 3e-3. At order 1 the same mesh reads about 8 % low.
The TW-MZM notebook's optical solve no longer pins order=1, so it takes the solve_modes default like the RF solve. The order parameter's docstring now describes order 2 as the general default rather than an RF-only choice. Two tests keep order=1 because their own checks depend on it, and each pin now says why. The staircase convergence test compares materials on one fixed mesh, so the order cancels out. The Palace cross-validation tolerance was set for femwell at order 1.
The Palace cross-validation test now solves femwell at the solve_modes default, order 2, instead of pinning order 1, and its n_eff tolerance is tightened from 1e-2 to 1e-3. On the test's mesh, femwell at order 2 agrees with Palace to 4.1e-5 relative on Re(n_eff) for the optical case and to 2.9e-4 on the complex n_eff for the RF materials. At order 1 both cases sit near 9e-3, just inside the old bound. The new bound keeps a margin of more than three over the measured agreement and rejects the order-1 result. These numbers were checked against Palace on the GDSFactory+ cloud on the same mesh. The test is marked palace_local and was not run locally.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
==========================================
- Coverage 91.16% 91.14% -0.02%
==========================================
Files 181 182 +1
Lines 25940 26119 +179
Branches 4897 4939 +42
==========================================
+ Hits 23647 23805 +158
- Misses 1317 1331 +14
- Partials 976 983 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Alisama20
left a comment
Alisama20
left a comment
There was a problem hiding this comment.
I ran the femwell tests (36 passed, Palace test skipped) and tests/common and tests/tcad (1205 passed). On a 500x220 nm Si strip against an h=0.01 order-2 reference, order 2 is better at equal cost. Order 1 at 6196 elements takes 1.0 s with an error of 1.6e-3. Order 2 at 1064 elements takes 0.58 s with an error of 6e-5. The 8 % and 4.3e-4 series-resistance numbers reproduce on the test mesh.
The "a few times more per solve" in the docstring is low. I measured 2.5x on a 94-element mesh and 8x to 12x above 1000 elements.
The new test docstring says refining at the strip does not move order 1. It does: -8.1 % goes to -12.6 % at 0.025. Order 2 at 0.025 reads -3.2e-3, just outside your 3e-3 bound, so the fix is partial.
The TW-MZM notebook uses the new default but the prose (9.7 GHz, 7.9, 12.7 and 17.6 dB/cm) is order 1. Please re-run it or say it is left out. I did not run it.
Correct two claims from review. Refining at the strip does move the order 1 reading (8.0 % low at 50 nm, 12.6 % low at 25 nm) and leaves order 2 3.2e-3 low at 25 nm: the remaining error comes from the coarse oxide, which a 0.5 um cap removes (order 2 at 7e-5). Say so in the solve_modes docstring and the test module, and replace the per-solve cost estimate with the measured range (about 2.5x on a hundred elements, about 10x above a thousand).
|
Thanks @Alisama20 for the measurements. You were right on both docstrings, fixed in ca55e7a.
One open point: an order-2 solve at 25 nm strip and 0.5 µm oxide (7,906 elements) returned a wildly wrong R' (relative error -2e7). It looks like the wrong mode was picked, but I have not confirmed that yet and will look into it. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca55e7aa28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| wavelength_um: float, | ||
| num_modes: int = 1, | ||
| order: int = 1, | ||
| order: int = 2, |
There was a problem hiding this comment.
When the published nbs/tcad_twmzm.ipynb workflow runs, its optical solve (lines 751-756) and RF sweep (lines 993-1000) both omit order, so this default change moves them from order 1 to order 2. The notebook itself is unchanged from the parent and still states the old fixed RF-index/impedance range and loss/bandwidth figures at lines 1036-1044 and 1286-1291; these are downstream of the series impedance that this change says is percent-low at order 1, so the generated values and explanatory text can no longer be treated as the same experiment. Re-execute and update the numerical narrative, or explicitly retain order 1 there.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
Alisama20
left a comment
Alisama20
left a comment
There was a problem hiding this comment.
I could not reproduce the -2e7 with 4 modes at 25 nm / 0.5 µm. The line mode is picked there and R′ reads -2.0e-4 at order 2.
It appears when more modes are requested. At 8 to 16 modes select_line_mode returns a spurious metal mode with |n_eff| of 1e4 to 5e4, at order 2 and also at order 1. These pass propagating_modes and win on the largest Re(n_eff), and R′ is then off by about 1e9.
So the cause is mode selection, not order 2. It needs its own issue.
The 1e-4 in the solve_modes docstring is optimistic. R′ moves by 2e-4 to 8e-4 with n_guess alone on the 25 nm / 0.5 µm mesh at order 2.
Alisama20
left a comment
Alisama20
left a comment
There was a problem hiding this comment.
The order-2 default is fine. tcad_twmzm and tcad_pn_junction run to the end on this branch, and the femwell tests pass (36).
The spurious-mode selection is separate from this PR.
|
Follow-up on the open point above: the order-2 solve at 25 nm strip / 0.5 µm oxide that gave R′ off by about -2e7 was a wrong mode pick. Asked for more modes, the solve also returns modes confined to the metal with |n_eff| near |sqrt(eps_metal)|, and |
Summary
gsim.femwell.solve_modesnow defaults to element order 2, for optical and RF solves. At order 1 the series impedance of a lossy metal line comes out percent low on a mesh that is fine at the conductors and coarse in the dielectric, and refining at the conductors does not fix it.gsim.femwellis not in 0.8.0, so the default changes without a deprecation period.order=1stays available for cheaper solves.Evidence
tests/femwell/test_series_resistance.py: a 2 x 0.5 um aluminium strip at 1 GHz against its DC resistance. Order 1 reads 8 % low; order 2 is within 4.3e-4.tests/femwell/test_cross_validation.pynow runs femwell at order 2 withN_EFF_RTOL = 1e-3. Against Palace on the same mesh (run on the GDSFactory+ cloud), order 2 agrees to 4.1e-5 (optical) and 2.9e-4 (RF materials); order 1 differed by about 9e-3. This test ispalace_localand was not run locally.Tests
Locally:
pytest tests/femwell36 passed, 2 deselected (palace_local).CI
In-fork CI on this branch: tests on Linux, macOS and Windows, pre-commit and coverage pass. Notebook execution and cloud smoke tests fail there only for lack of the GDSFactory+ API key.