Bump tidy3d to version >= 2.10 + fix a few bugs in meow making coverage testing fail#698
Merged
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Contributor
Reviewer's GuideUpdates the project to require tidy3d>=2.10, migrates Tidy3D integration to newer APIs (ModalComponentModeler, Complex), fixes serialization and MEOW EME integration issues that were breaking tests, and refreshes docs for installing optional dependency groups with uv. Sequence diagram for write_sparameters using ModalComponentModeler and web.runsequenceDiagram
participant User
participant Tidy3dComponent as Tidy3dComponent
participant ModalComponentModeler as ModalComponentModeler
participant WebClient as web
User->>Tidy3dComponent: write_sparameters(component, settings)
Tidy3dComponent->>Tidy3dComponent: get_component_modeler(...)
Tidy3dComponent-->>ModalComponentModeler: construct simulation, ports, freqs
Tidy3dComponent-->>User: modeler
User->>WebClient: web.run(modeler, task_name, verbose, path)
WebClient->>ModalComponentModeler: read simulation and ports
WebClient-->>User: modeler_data (Sparameters and results)
Class diagram for updated Tidy3D Waveguide model and serializerclassDiagram
class BaseModel
class Waveguide {
<<pydantic_model>>
+arbitrary_types_allowed bool
}
class td_Medium
class td_CustomMedium
class td_PoleResidue
class Serializer {
+custom_serializer(data)
}
BaseModel <|-- Waveguide
Serializer --> BaseModel : serialize_json
Serializer --> td_Medium : serialize_json
Serializer --> td_CustomMedium : serialize_json
Serializer --> td_PoleResidue : serialize_json
Class diagram for ModalComponentModeler migration and materials APIclassDiagram
class Tidy3dComponent {
+get_component_modeler(wavelength, bandwidth, num_freqs, min_steps_per_wvl, center_z, sim_size_z, port_size_mult, run_only, element_mappings, extra_monitors, mode_spec, boundary_spec, run_time, shutoff, grid_eps, symmetry, kwargs) ModalComponentModeler
}
class ModalComponentModeler
class MaterialsAPI {
+get_epsilon(spec, wavelength) Complex
}
class Complex
Tidy3dComponent --> ModalComponentModeler : builds
MaterialsAPI --> Complex : returns
Flow diagram for updated MEOW EME S-matrix computationflowchart TD
A[modes_per_cell, cells] --> B[compute_propagation_s_matrices]
A --> C[compute_interface_s_matrices]
B --> D[build net dict]
C --> D
D --> E[_get_netlist]
E --> F[net instances to sax.scoo]
F --> G[connections_or_nets]
G --> H{is dict?}
H -- yes --> I["build nets list from key,value"]
H -- no --> J[use existing nets]
I --> K["net nets = nets list"]
J --> K
K --> L["sa = sax.circuit(net)"]
L --> M[return S matrix]
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
custom_serializerfunction usesisinstancewith a union type (str | None | np.ndarrayandBaseModel | td.CustomMedium | td.Medium | td.PoleResidue), which will raise aTypeErrorat runtime; these should be changed to tuples likeisinstance(data, (str, type(None), np.ndarray))andisinstance(data, (BaseModel, td.CustomMedium, td.Medium, td.PoleResidue)). - The
get_component_modelerdocstring still documentsfolder_name,path_dir, andverbosearguments that are no longer in the function signature; either reintroduce these parameters if they are still needed or update the docstring and any call sites to keep the API and documentation consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `custom_serializer` function uses `isinstance` with a union type (`str | None | np.ndarray` and `BaseModel | td.CustomMedium | td.Medium | td.PoleResidue`), which will raise a `TypeError` at runtime; these should be changed to tuples like `isinstance(data, (str, type(None), np.ndarray))` and `isinstance(data, (BaseModel, td.CustomMedium, td.Medium, td.PoleResidue))`.
- The `get_component_modeler` docstring still documents `folder_name`, `path_dir`, and `verbose` arguments that are no longer in the function signature; either reintroduce these parameters if they are still needed or update the docstring and any call sites to keep the API and documentation consistent.
## Individual Comments
### Comment 1
<location path="gplugins/tidy3d/component.py" line_range="239-248" />
<code_context>
+ """Returns a ModalComponentModeler instance for the component.
</code_context>
<issue_to_address>
**issue:** Docstring still documents folder_name, path_dir, and verbose parameters that are no longer in the function signature.
Please update the docstring so its parameter list matches the current function signature—either remove `folder_name`, `path_dir`, and `verbose`, or add the parameters back if they’re still meant to be supported.
</issue_to_address>
### Comment 2
<location path="gplugins/tidy3d/modes.py" line_range="38-41" />
<code_context>
-def custom_serializer(data: str | float | BaseModel) -> str:
+def custom_serializer(data: str | float | BaseModel | td.Medium) -> str:
# If data is a string, just return it.
if isinstance(data, str | None | np.ndarray):
</code_context>
<issue_to_address>
**issue (bug_risk):** Using union types directly in isinstance will raise a TypeError at runtime.
`isinstance` doesn’t accept PEP 604 unions. This call will raise `TypeError: isinstance() argument 2 cannot be a union`. Use a tuple of types instead, e.g. `isinstance(data, (BaseModel, td.CustomMedium, td.Medium, td.PoleResidue))` to preserve the same behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+38
to
41
| def custom_serializer(data: str | float | BaseModel | td.Medium) -> str: | ||
| # If data is a string, just return it. | ||
| if isinstance(data, str | None | np.ndarray): | ||
| return data |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using union types directly in isinstance will raise a TypeError at runtime.
isinstance doesn’t accept PEP 604 unions. This call will raise TypeError: isinstance() argument 2 cannot be a union. Use a tuple of types instead, e.g. isinstance(data, (BaseModel, td.CustomMedium, td.Medium, td.PoleResidue)) to preserve the same behavior.
joamatab
approved these changes
May 5, 2026
tidy3d often introduces breaking API changes across minor releases, so we pin to ~=2.11.1 (>=2.11.1, <2.12.0) to avoid unexpected breakage.
Contributor
|
tidy3d often introduces breaking API changes across minor releases, hence the ~=2.11.1 pin to cap at <2.12.0. |
Contributor
Author
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
2 participants
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.
Breaking changes
tidy3dto >=2.10Fixes
gplugins/meow/meow_eme.py,_compute_s_matrix(modes_per_cell, cells)was:net["connections"]when the correct field name isnet["nets"].gplugins/tidy3d/modes.py,custom_serializer()couldn't handle tidy3d Medium objects, causing an error when instanciating aWaveguideobject with td Mediums as arguments.gplugins/tidy3d/materials.py,ComplexNumberis deprecated and replaced byComplex.Refactors
tidy3d.plugins.smatrix.ComponentModeleris deprecated ->tidy3d.plugins.smatrix.ModalComponentModeler.gdsfactory.generic_techis deprecated ->gdsfactory.gpdkDocs
uv syncRelated issues
Closes #697
Summary by Sourcery
Update tidy3d- and meow-related integrations to support tidy3d>=2.10 and newer meow APIs while aligning type usage and documentation.
Bug Fixes:
Enhancements:
Build:
Documentation: