Upload, but not run, simulation for write_sparameters routine#728
Upload, but not run, simulation for write_sparameters routine#728jackgdsf wants to merge 5 commits into
Conversation
…ows used to run from GUI for safe measure
# Conflicts: # gplugins/tidy3d/component.py
jackgdsf
requested review from
ThomasPluck,
joamatab and
nikosavola
as code owners
Reviewer's GuideAdds an Sequence diagram for write_sparameters upload_only behaviorsequenceDiagram
actor User
participant write_sparameters
participant modeler as ComponentModeler
participant web
participant webapi
User->>write_sparameters: write_sparameters(..., upload_only=True)
write_sparameters->>modeler: simulation
alt upload_only
write_sparameters->>modeler: to_source(port, mode_index=0)
write_sparameters->>modeler: to_monitor(port)
write_sparameters->>modeler: simulation.copy(update={sources, monitors})
write_sparameters->>webapi: upload(sim_with_sources, task_name)
webapi-->>write_sparameters: task_handle
write_sparameters-->>User: task_handle
else run_simulation
write_sparameters->>web: run(modeler, task_name, verbose, path)
web-->>write_sparameters: modeler_data
write_sparameters->>write_sparameters: smatrix()
write_sparameters->>write_sparameters: compute_sparameters()
write_sparameters->>write_sparameters: save npz(filepath)
write_sparameters-->>User: sparameters
end
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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="gplugins/tidy3d/component.py" line_range="442-445" />
<code_context>
plot_epsilon: bool = False,
filepath: PathType | None = None,
overwrite: bool = False,
+ upload_only: bool = False,
**kwargs: Any,
) -> Sparameters:
</code_context>
<issue_to_address>
**issue (bug_risk):** When `upload_only=True`, the function returns a different type than in the normal path.
The `upload_only` branch returns `webapi.upload(...)`, which likely differs from the annotated `Sparameters` return type and from the dict-like S-parameter object returned in the normal path. This divergence can surprise callers and cause runtime errors. Please either keep the return type consistent (e.g., still return `sp` or a dedicated wrapper type) or split the upload behavior into a separate function with its own return annotation.
</issue_to_address>
### Comment 2
<location path="gplugins/tidy3d/component.py" line_range="587-596" />
<code_context>
+ if upload_only:
</code_context>
<issue_to_address>
**question (bug_risk):** `upload_only` is ignored when an S-parameter file already exists, which may be surprising.
Because this branch is only entered in the `else` of `if filepath.exists()`, setting `upload_only=True` has no effect when the file already exists and `overwrite=False`—the function just returns cached data. If `upload_only` is meant to always trigger a cloud upload, consider checking it before returning cached results, or clarify in the docs that it is ignored when a cache is present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| upload_only: bool = False, | ||
| **kwargs: Any, | ||
| ) -> Sparameters: | ||
| """Writes the S-parameters for a component. |
There was a problem hiding this comment.
issue (bug_risk): When upload_only=True, the function returns a different type than in the normal path.
The upload_only branch returns webapi.upload(...), which likely differs from the annotated Sparameters return type and from the dict-like S-parameter object returned in the normal path. This divergence can surprise callers and cause runtime errors. Please either keep the return type consistent (e.g., still return sp or a dedicated wrapper type) or split the upload behavior into a separate function with its own return annotation.
| if upload_only: | ||
| plot_sources = [ | ||
| modeler.to_source(port=p, mode_index=0) for p in modeler.ports | ||
| ] | ||
| plot_monitors = [modeler.to_monitor(port=p) for p in modeler.ports] | ||
|
|
||
| cz = c.get_layer_center("core")[2] | ||
| birdseye = FieldMonitor( | ||
| name="birdseye", | ||
| interval_space=(4, 4, 1), |
There was a problem hiding this comment.
question (bug_risk): upload_only is ignored when an S-parameter file already exists, which may be surprising.
Because this branch is only entered in the else of if filepath.exists(), setting upload_only=True has no effect when the file already exists and overwrite=False—the function just returns cached data. If upload_only is meant to always trigger a cloud upload, consider checking it before returning cached results, or clarify in the docs that it is ignored when a cache is present.
![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 introduces an upload_only option to write_sparameters in gplugins/tidy3d/component.py, allowing users to configure and upload simulations to the cloud without running them locally. The feedback identifies three important improvements: dynamically retrieving the z-center from the simulation instead of hardcoding the "core" layer to prevent potential crashes, explicitly passing both task_name and folder_name to webapi.upload to preserve unique task names, and updating the return type annotation of write_sparameters to Sparameters | str to reflect the new upload-only return value.
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.
| cz = c.get_layer_center("core")[2] | ||
| birdseye = FieldMonitor( | ||
| name="birdseye", | ||
| interval_space=(4, 4, 1), | ||
| freqs=td.constants.C_0 / wavelength, | ||
| center=(c.center[0], c.center[1], cz), | ||
| size=(c.size[0], c.size[1], 0), | ||
| ) |
There was a problem hiding this comment.
Hardcoding the layer name "core" to find the z-center will cause a KeyError or crash for components or layer stacks that do not define a "core" layer. Instead, we can dynamically retrieve the z-center directly from the simulation's center (modeler.simulation.center[2]). Additionally, the freqs parameter in FieldMonitor should be wrapped in a list/tuple to prevent potential validation errors in tidy3d, and we should use the simulation's center and size to ensure the birdseye monitor perfectly spans the simulation domain.
| cz = c.get_layer_center("core")[2] | |
| birdseye = FieldMonitor( | |
| name="birdseye", | |
| interval_space=(4, 4, 1), | |
| freqs=td.constants.C_0 / wavelength, | |
| center=(c.center[0], c.center[1], cz), | |
| size=(c.size[0], c.size[1], 0), | |
| ) | |
| cz = modeler.simulation.center[2] | |
| birdseye = FieldMonitor( | |
| name="birdseye", | |
| interval_space=(4, 4, 1), | |
| freqs=[td.constants.C_0 / wavelength], | |
| center=(modeler.simulation.center[0], modeler.simulation.center[1], cz), | |
| size=(modeler.simulation.size[0], modeler.simulation.size[1], 0), | |
| ) |
| } | ||
| ) | ||
|
|
||
| s = webapi.upload(sim_with_sources, task_name=folder_name) |
There was a problem hiding this comment.
In webapi.upload, the second positional argument is task_name. By passing task_name=folder_name, the uploaded task will be named "default" (or whatever folder_name is set to) on the Tidy3D platform, and the unique task_name (hash) will be lost. We should pass task_name=task_name and folder_name=folder_name explicitly to ensure the task is named uniquely and placed in the correct folder.
| s = webapi.upload(sim_with_sources, task_name=folder_name) | |
| s = webapi.upload(sim_with_sources, task_name=task_name, folder_name=folder_name) |
| overwrite: bool = False, | ||
| upload_only: bool = False, | ||
| **kwargs: Any, | ||
| ) -> Sparameters: |
There was a problem hiding this comment.
The return type annotation of write_sparameters is currently Sparameters (which is a dictionary). However, when upload_only is True, the function returns the task ID/handle (a string) from webapi.upload. To ensure type safety and correct static analysis, the return type annotation should be updated to Sparameters | str.
| ) -> Sparameters: | |
| ) -> Sparameters | str: |
2 participants
Supersedes #664 — same changes with merge conflicts resolved against current
main.Original PR by @heydaridlm. This PR is opened from a fork because the original branch needed a merge conflict resolution and we didn't have push access to the upstream branch.
Original description:
In the
write_sparametersroutine, a simulation is created and automatically deployed/run on tidy servers. I think it is much safer to have an option to upload the simulation only, then let the user go into the Tidy Website GUI and inspect the simulation by hand to see if things make sense, then let the user run the simulation from the Tidy GUI. I also added a 'birdseye' monitor that spans the XY domain of the simulation and slices through the middle of the "core" of the component of interest in Z.Todo: In doing this, I had to comment out the part where the s parameters are returned, this needs to be fixed, otherwise
write_sparametersdoes not return anything except for the whether or not theuploadwas successful.Summary by Sourcery
Add an option for write_sparameters to upload simulations without running them and introduce a diagnostic birdseye monitor for inspection in Tidy3D.
New Features:
Enhancements: