feat: allow custom GridSpec in tidy3d write_sparameters#729
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adds an optional Sequence diagram for grid_spec handling in write_sparameterssequenceDiagram
participant Caller
participant write_sparameters
participant get_component_modeler
participant td_GridSpec as td.GridSpec
Caller->>write_sparameters: write_sparameters(..., grid_spec)
write_sparameters->>get_component_modeler: get_component_modeler(..., grid_spec)
alt grid_spec is not None
get_component_modeler->>get_component_modeler: use provided grid_spec
else grid_spec is None
get_component_modeler->>td_GridSpec: auto(wavelength, min_steps_per_wvl)
td_GridSpec-->>get_component_modeler: GridSpec
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Label error. Requires at least 1 of: breaking, bug, github_actions, documentation, dependencies, enhancement, feature, maintenance, security. Found: |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
write_sparameters_batch,grid_specis documented but not added to the function signature or passed through toget_component_modeler, which will confuse users expecting it to work; consider wiring the parameter through consistently or removing it from the docstring. - Silently ignoring
min_steps_per_wvlwhengrid_specis provided may be surprising; consider adding a small runtime check or warning when both are passed to make this behavior explicit to callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `write_sparameters_batch`, `grid_spec` is documented but not added to the function signature or passed through to `get_component_modeler`, which will confuse users expecting it to work; consider wiring the parameter through consistently or removing it from the docstring.
- Silently ignoring `min_steps_per_wvl` when `grid_spec` is provided may be surprising; consider adding a small runtime check or warning when both are passed to make this behavior explicit to callers.
## Individual Comments
### Comment 1
<location path="gplugins/tidy3d/component.py" line_range="245" />
<code_context>
wavelength: The wavelength for the ModalComponentModeler. Defaults to 1.55.
bandwidth: The bandwidth for the ModalComponentModeler. Defaults to 0.2.
num_freqs: The number of frequencies for the ModalComponentModeler. Defaults to 21.
- min_steps_per_wvl: The minimum number of steps per wavelength for the ModalComponentModeler. Defaults to 30.
+ min_steps_per_wvl: The minimum number of steps per wavelength for the ModalComponentModeler. Defaults to 30. Ignored when grid_spec is provided.
</code_context>
<issue_to_address>
**issue:** Docstring default for `num_freqs` is inconsistent with the function signature.
In `get_component_modeler`, the parameter `num_freqs` defaults to `6`, but the docstring still says `Defaults to 21.` Please update the docstring so it matches the actual default value.
</issue_to_address>
### Comment 2
<location path="gplugins/tidy3d/component.py" line_range="281-283" />
<code_context>
)
- grid_spec = td.GridSpec.auto(
+ grid_spec = grid_spec or td.GridSpec.auto(
wavelength=wavelength, min_steps_per_wvl=min_steps_per_wvl
)
</code_context>
<issue_to_address>
**suggestion:** Use an explicit `is None` check for `grid_spec` instead of `grid_spec or ...`.
Using `grid_spec or td.GridSpec.auto(...)` depends on `grid_spec` always being truthy. If `GridSpec` (or a wrapper) ever implements a falsy `__bool__`/`__len__`, the provided spec would be incorrectly replaced. An explicit `if grid_spec is None` check avoids this potential misbehaviour and is safer.
```suggestion
if grid_spec is None:
grid_spec = td.GridSpec.auto(
wavelength=wavelength, min_steps_per_wvl=min_steps_per_wvl
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a grid_spec parameter to get_component_modeler and write_sparameters in gplugins/tidy3d/component.py, allowing users to provide a custom grid specification for the simulation mesh. Feedback on this change highlights that the custom grid_spec is not forwarded to the get_component_modeler call used for plotting the simulation, meaning the plotted simulation will not accurately reflect the custom grid configuration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Adds an optional
grid_spec: td.GridSpec | Noneparameter toTidy3DComponent.get_component_modeler,write_sparameters, andwrite_sparameters_batch, giving full control over the simulation mesh (e.g.td.GridSpec.uniform(dl=20*nm)or per-axisAutoGrid).When
grid_specisNone(default), behavior is unchanged: it falls back totd.GridSpec.auto(wavelength=wavelength, min_steps_per_wvl=min_steps_per_wvl).min_steps_per_wvlis ignored whengrid_specis provided.Summary by Sourcery
Add support for user-provided Tidy3D GridSpec when generating component modelers and writing s-parameters, while preserving existing auto grid behavior by default.
New Features:
Enhancements: