add target_neff parameter to mode solver#716
Conversation
Reviewer's GuideAdds configurability to the Tidy3D mode solver via an optional target effective index, introduces mode tracking in bend mismatch sweeps, and switches the default SiO2 cladding material to a lossless model with corresponding test update. 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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
sweep_bend_mismatch, thetrack_modesvalidation currently requireswaveguide.num_modes >= 2even ifmodes_to_trackonly contains a single mode (e.g.(0,)), which seems unnecessarily restrictive; consider relaxing this check to require onlywaveguide.num_modes > max(modes_to_track). - The
track_modesargument docs saymodes_to_trackis required whentrack_modesis True, butmodes_to_trackhas a non-empty default; either adjust the docstring or simplify thelen(modes_to_track) == 0check to match the intended usage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `sweep_bend_mismatch`, the `track_modes` validation currently requires `waveguide.num_modes >= 2` even if `modes_to_track` only contains a single mode (e.g. `(0,)`), which seems unnecessarily restrictive; consider relaxing this check to require only `waveguide.num_modes > max(modes_to_track)`.
- The `track_modes` argument docs say `modes_to_track` is required when `track_modes` is True, but `modes_to_track` has a non-empty default; either adjust the docstring or simplify the `len(modes_to_track) == 0` check to match the intended usage.
## Individual Comments
### Comment 1
<location path="gplugins/tidy3d/modes.py" line_range="897-906" />
<code_context>
+ modes_to_track: straight mode indices to track. Required when
+ track_modes is True.
"""
+ if track_modes:
+ if len(modes_to_track) == 0:
+ raise ValueError("modes_to_track must be provided when track_modes is True")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Revisit squaring strategy to avoid double-squaring and align with the documented quantity.
In the `track_modes` branch you compute `best = [np.max(np.abs(overlap[:, m]) ** 2) ...]` and later return `np.abs(results) ** 2`, so the output is effectively `|overlap|**4`. This matches the non-tracking branch, but the docstring says it returns the “overlap integral squared”. Please decide whether the intended quantity is `|overlap|**2` or `|overlap|**4` and update both branches consistently. If only the squared magnitude is intended, remove the inner `** 2` (and possibly the final `** 2`) to avoid redundant work and mismatch with the documentation.
Suggested implementation:
```python
# Take the maximum absolute overlap for each tracked straight mode.
# The squaring to obtain |overlap|**2 is applied once at the end of the function.
best = [np.max(np.abs(overlap[:, m]), axis=1) for m in modes_to_track]
```
```python
# Take the maximum absolute overlap across all modes; squaring is done once at the end.
results = np.max(np.abs(overlap), axis=1)
```
```python
# Return the overlap integral squared, i.e. |overlap|**2, in line with the docstring.
return np.abs(results) ** 2
```
These edits assume:
1. The tracked-modes branch contains a line like:
`best = [np.max(np.abs(overlap[:, m]) ** 2, axis=1) for m in modes_to_track]`
2. The non-tracking branch contains a line like:
`results = np.max(np.abs(overlap) ** 2, axis=1)`
3. The function currently ends with `return np.abs(results) ** 2`.
If the actual code uses slightly different variable names or shapes (e.g. `axis=0` vs `axis=1`, or builds `results` from `best` before returning), you should:
- Remove any inner `** 2` that is applied directly to `np.abs(overlap...)` in both the tracking and non-tracking branches.
- Keep a single squaring step right before returning, so the final returned quantity is `|overlap|**2`, matching the docstring “Overlap integral squared”.
- Ensure intermediate comments (if any) do not claim `|overlap|**4`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if track_modes: | ||
| if len(modes_to_track) == 0: | ||
| raise ValueError("modes_to_track must be provided when track_modes is True") | ||
| if waveguide.num_modes < max(modes_to_track) + 1: | ||
| raise ValueError( | ||
| f"num_modes ({waveguide.num_modes}) must be >= " | ||
| f"{max(modes_to_track) + 1} to track modes {modes_to_track}" | ||
| ) | ||
| if waveguide.num_modes < 2: | ||
| raise ValueError("Track modes requires num_modes >= 2") |
There was a problem hiding this comment.
suggestion (bug_risk): Revisit squaring strategy to avoid double-squaring and align with the documented quantity.
In the track_modes branch you compute best = [np.max(np.abs(overlap[:, m]) ** 2) ...] and later return np.abs(results) ** 2, so the output is effectively |overlap|**4. This matches the non-tracking branch, but the docstring says it returns the “overlap integral squared”. Please decide whether the intended quantity is |overlap|**2 or |overlap|**4 and update both branches consistently. If only the squared magnitude is intended, remove the inner ** 2 (and possibly the final ** 2) to avoid redundant work and mismatch with the documentation.
Suggested implementation:
# Take the maximum absolute overlap for each tracked straight mode.
# The squaring to obtain |overlap|**2 is applied once at the end of the function.
best = [np.max(np.abs(overlap[:, m]), axis=1) for m in modes_to_track] # Take the maximum absolute overlap across all modes; squaring is done once at the end.
results = np.max(np.abs(overlap), axis=1) # Return the overlap integral squared, i.e. |overlap|**2, in line with the docstring.
return np.abs(results) ** 2These edits assume:
- The tracked-modes branch contains a line like:
best = [np.max(np.abs(overlap[:, m]) ** 2, axis=1) for m in modes_to_track] - The non-tracking branch contains a line like:
results = np.max(np.abs(overlap) ** 2, axis=1) - The function currently ends with
return np.abs(results) ** 2.
If the actual code uses slightly different variable names or shapes (e.g. axis=0 vs axis=1, or builds results from best before returning), you should:
- Remove any inner
** 2that is applied directly tonp.abs(overlap...)in both the tracking and non-tracking branches. - Keep a single squaring step right before returning, so the final returned quantity is
|overlap|**2, matching the docstring “Overlap integral squared”. - Ensure intermediate comments (if any) do not claim
|overlap|**4.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the default SiO2 material to Palik_Lossless and introduces a target_neff parameter for the mode solver in Waveguide and WaveguideCoupler classes. It also adds mode tracking functionality to the sweep_bend_mismatch function. The review feedback highlights a potential logic error involving double-squaring of the overlap values and suggests removing an unnecessary restriction that requires multiple modes for tracking.
| ) | ||
|
|
||
| if track_modes: | ||
| best = [np.max(np.abs(overlap[:, m]) ** 2) for m in modes_to_track] |
There was a problem hiding this comment.
There is a potential logic issue with double squaring when track_modes is enabled.
Line 918 calculates the power overlap as np.abs(overlap) ** 2. Then, line 925 squares the entire results list again: return np.abs(results) ** 2. This results in returning the overlap to the fourth power (
While the docstring mentions the loss is squared for a round trip, the naming 'Overlap integral squared' usually implies else branch. However, if the intention was to return the power overlap
| if waveguide.num_modes < 2: | ||
| raise ValueError("Track modes requires num_modes >= 2") |
There was a problem hiding this comment.
The restriction waveguide.num_modes < 2 for mode tracking seems unnecessary. Even if a waveguide has only one mode, a user might want to use the track_modes logic for consistency in their scripts, and the underlying overlap calculation overlap[:, m] works correctly for a single mode (where m=0).
2 participants
Summary
target_neffparameter toWaveguidemodel to control the target effective index used by the mode solver (relevant for bent and leaky modes)track_modesoption tosweep_bend_mismatchfor tracking specific straight modes across bend radiiPalik_Lossless) as default cladding materialTest plan
Summary by Sourcery
Add configurability to the mode solver and bend mismatch analysis while updating default materials and tests.
New Features:
Enhancements:
Tests: