Update tidy3d API calls for web and ModalComponentModeler#676
Open
jonbenfri wants to merge 4 commits intogdsfactory:mainfrom
Open
Update tidy3d API calls for web and ModalComponentModeler#676jonbenfri wants to merge 4 commits intogdsfactory:mainfrom
ModalComponentModeler#676jonbenfri wants to merge 4 commits intogdsfactory:mainfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the Tidy3D integration to align with the newer web-based API and relaxes the default mode specification by removing polarization filtering, affecting both component model construction and S-parameter extraction helpers. Sequence diagram for updated write_sparameters Tidy3D web workflowsequenceDiagram
actor UserCode
participant WriteSparameters as write_sparameters
participant ComponentModeler
participant FileSystem
participant Tidy3DWebAPI as Tidy3D_web
UserCode->>WriteSparameters: call write_sparameters(component, ports, freqs, ...)
WriteSparameters->>ComponentModeler: get_component_modeler(..., mode_spec, boundary_spec, ...)
ComponentModeler-->>WriteSparameters: return modeler
WriteSparameters->>FileSystem: check for cached sparameters file
alt cache_exists
FileSystem-->>WriteSparameters: load numpy archive
WriteSparameters-->>UserCode: return cached sparameters
else cache_missing
WriteSparameters->>Tidy3DWebAPI: web.run(modeler, verbose, path)
Tidy3DWebAPI-->>WriteSparameters: modeler_data
WriteSparameters->>WriteSparameters: s = modeler_data.smatrix()
WriteSparameters->>FileSystem: save sparameters to numpy archive
WriteSparameters-->>UserCode: return computed sparameters
end
Flow diagram for write_sparameters with caching and web.runflowchart TD
A[Start write_sparameters] --> B[Build ComponentModeler via get_component_modeler]
B --> C[Compute cache filepath]
C --> D{Cached file exists?}
D -->|Yes| E[Load sparameters from NPZ]
E --> F[Return cached sparameters]
D -->|No| G["Call web.run(modeler, verbose, path)"]
G --> H[Get modeler_data]
H --> I[Extract smatrix from modeler_data.smatrix]
I --> J[Iterate ports and modes to build sparameters dict]
J --> K[Save sparameters dict to NPZ]
K --> L[Return computed sparameters]
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
folder_nameargument towrite_sparametersis no longer used after removing it from theComponentModelerconstruction and theupdated_copycall; consider dropping it from the signature or wiring it into the newweb.runpath handling to avoid confusion. - Switching the default
mode_specfromtd.ModeSpec(num_modes=1, filter_pol="te")totd.ModeSpec(num_modes=1)changes the default modal behavior; if this is intentional, consider checking for call sites that might implicitly rely on TE filtering and updating them for clarity. - With the move to
web.run(modeler, verbose=verbose, path=dirpath / "simulation.hdf5"), ensure thatdirpathis consistently aPath(or convert explicitly) so that path joining and downstream file handling remain robust across different call patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `folder_name` argument to `write_sparameters` is no longer used after removing it from the `ComponentModeler` construction and the `updated_copy` call; consider dropping it from the signature or wiring it into the new `web.run` path handling to avoid confusion.
- Switching the default `mode_spec` from `td.ModeSpec(num_modes=1, filter_pol="te")` to `td.ModeSpec(num_modes=1)` changes the default modal behavior; if this is intentional, consider checking for call sites that might implicitly rely on TE filtering and updating them for clarity.
- With the move to `web.run(modeler, verbose=verbose, path=dirpath / "simulation.hdf5")`, ensure that `dirpath` is consistently a `Path` (or convert explicitly) so that path joining and downstream file handling remain robust across different call patterns.
## Individual Comments
### Comment 1
<location> `gplugins/tidy3d/component.py:528` </location>
<code_context>
path_dir = pathlib.Path(dirpath) / modeler._hash_self()
- modeler = modeler.updated_copy(path_dir=str(path_dir))
+ modeler = modeler.updated_copy()
sp = {}
</code_context>
<issue_to_address>
**issue:** `path_dir` is computed but no longer used, which suggests either dead code or an incomplete refactor.
After switching to `modeler.updated_copy()` with no arguments, `path_dir` is no longer referenced. If the hashed directory behavior is being removed, `path_dir` should likely be deleted. If that behavior is still needed (for organization/caching), `updated_copy` should probably still receive `path_dir` (or the new target directory) as an argument.
</issue_to_address>
### Comment 2
<location> `gplugins/tidy3d/component.py:590-591` </location>
<code_context>
else:
time.sleep(0.2)
- s = modeler.run()
+ modeler_data = web.run(modeler, verbose=verbose, path=dirpath / "simulation.hdf5")
+ s = modeler_data.smatrix()
for port_in in s.port_in.values:
for port_out in s.port_out.values:
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `dirpath / "simulation.hdf5"` assumes `dirpath` is a `Path`, which may be a regression if callers pass a string.
Previously we only wrapped `dirpath` in `Path` when building `path_dir`, so callers could still pass a plain string. With `web.run(..., path=dirpath / "simulation.hdf5")`, a string `dirpath` will now raise a `TypeError`. Please either normalize `dirpath` once at the top (e.g. `dirpath = pathlib.Path(dirpath)`) or change this call to `path=pathlib.Path(dirpath) / "simulation.hdf5"` to handle both strings and `Path` objects safely.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| path_dir = pathlib.Path(dirpath) / modeler._hash_self() | ||
| modeler = modeler.updated_copy(path_dir=str(path_dir)) | ||
| modeler = modeler.updated_copy() |
Contributor
There was a problem hiding this comment.
issue: path_dir is computed but no longer used, which suggests either dead code or an incomplete refactor.
After switching to modeler.updated_copy() with no arguments, path_dir is no longer referenced. If the hashed directory behavior is being removed, path_dir should likely be deleted. If that behavior is still needed (for organization/caching), updated_copy should probably still receive path_dir (or the new target directory) as an argument.
Comment on lines
+590
to
+591
| modeler_data = web.run(modeler, verbose=verbose, path=dirpath / "simulation.hdf5") | ||
| s = modeler_data.smatrix() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using dirpath / "simulation.hdf5" assumes dirpath is a Path, which may be a regression if callers pass a string.
Previously we only wrapped dirpath in Path when building path_dir, so callers could still pass a plain string. With web.run(..., path=dirpath / "simulation.hdf5"), a string dirpath will now raise a TypeError. Please either normalize dirpath once at the top (e.g. dirpath = pathlib.Path(dirpath)) or change this call to path=pathlib.Path(dirpath) / "simulation.hdf5" to handle both strings and Path objects safely.
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
1 participant
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.
Solves issue with stale tidy3d notebook example code
notebooks/workflow_3_cascaded_mzi.ipynb(issue #674) after adding in solution to activate PDK (issue #673 with related pull request #675):Summary by Sourcery
Update tidy3d component modeling and S-parameter utilities to align with the latest web-based API and defaults.
Bug Fixes:
Enhancements: