Upload, but not run, simulation for write_sparameters routine #664
Conversation
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
…ows used to run from GUI for safe measure
Contributor
Reviewer's GuideModify write_sparameters to upload a modified simulation with visualization monitors to Tidy web instead of running it locally and computing S-parameters, returning the uploaded task object instead of S-parameter data. Sequence diagram for updated write_sparameters upload-only workflowsequenceDiagram
actor User
participant write_sparameters
participant Modeler
participant Component_c
participant Web
User->>write_sparameters: call write_sparameters(...)
alt cached_sparameters_exist
write_sparameters-->>User: return loaded_sparameters_dict
else no_cached_sparameters
write_sparameters->>Modeler: modeler.ports
write_sparameters->>Modeler: to_source(port, mode_index_0) for each port
Modeler-->>write_sparameters: plot_sources
write_sparameters->>Modeler: to_monitor(port) for each port
Modeler-->>write_sparameters: plot_monitors
write_sparameters->>Component_c: get_layer_center(core)
Component_c-->>write_sparameters: core_center
write_sparameters->>Component_c: center, size
Component_c-->>write_sparameters: center, size
write_sparameters->>write_sparameters: create FieldMonitor birdseye
write_sparameters->>Modeler: modeler.simulation
Modeler-->>write_sparameters: simulation
write_sparameters->>write_sparameters: create sim_with_sources (sources, monitors + birdseye)
write_sparameters->>Web: upload(sim_with_sources, task_name=folder_name)
Web-->>write_sparameters: task_s
write_sparameters-->>User: return task_s
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}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The behavior of
write_sparametershas changed from returning an s‑parameter dict to returning the upload handle, which is a breaking API change; consider adding anupload_only(or similar) option and preserving the original return path when not set. - The commented-out s-parameter extraction block after
return sis now unreachable and partially inverted (if s.status != "completed"); if you intend to reuse it, refactor it into a helper and call it based ons.status == "completed"rather than leaving dead code commented out. - The new
birdseyemonitor construction assumes availability ofc,FieldMonitor,td, andwavelengthin scope; it would be more robust to pass these in or derive them locally sowrite_sparametersdoes not depend on implicit external state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The behavior of `write_sparameters` has changed from returning an s‑parameter dict to returning the upload handle, which is a breaking API change; consider adding an `upload_only` (or similar) option and preserving the original return path when not set.
- The commented-out s-parameter extraction block after `return s` is now unreachable and partially inverted (`if s.status != "completed"`); if you intend to reuse it, refactor it into a helper and call it based on `s.status == "completed"` rather than leaving dead code commented out.
- The new `birdseye` monitor construction assumes availability of `c`, `FieldMonitor`, `td`, and `wavelength` in scope; it would be more robust to pass these in or derive them locally so `write_sparameters` does not depend on implicit external state.
## Individual Comments
### Comment 1
<location> `gplugins/tidy3d/component.py:602` </location>
<code_context>
+ 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),freqs=td.C_0 / wavelength, center=(c.center[0], c.center[1], cz), size=(c.size[0], c.size[1], 0))
+
+ sim_with_sources = modeler.simulation.copy(
</code_context>
<issue_to_address>
**issue (bug_risk):** Use a consistent speed-of-light constant to avoid subtle unit or API issues.
Elsewhere in this function `td.constants.C_0` is used, but here `td.C_0` is referenced. If `td.C_0` isn’t defined or differs from `td.constants.C_0`, this could cause runtime errors or inconsistent frequency scaling. Please use a single, consistent constant here (e.g. `td.constants.C_0 / wavelength`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
The failure in the autocheck appears to be a misunderstanding of a divide by zero which is impossible as frequency != 0, ever. |
There was a problem hiding this comment.
Pull request overview
This PR adds an upload_only option to the write_sparameters function, allowing users to upload simulations to Tidy3D servers without automatically running them. This enables manual inspection via the Tidy3D GUI before execution.
Key Changes:
- Added
upload_onlyparameter to create and upload simulations without running them - Added a "birdseye" field monitor for visualization spanning the XY domain
- Updated code to use
td.constants.C_0instead oftd.C_0for consistency
Comments suppressed due to low confidence (1)
gplugins/tidy3d/component.py:498
- The docstring is missing documentation for the new
upload_onlyparameter. This parameter should be documented in the Args section, explaining that when True, the simulation is uploaded to Tidy3D servers without running, and the function returns the upload task handle instead of S-parameters.
"""Writes the S-parameters for a component.
Args:
component: gdsfactory component to write the S-parameters for.
layer_stack: The layer stack for the component. If None, uses active pdk layer_stack.
material_mapping: A mapping of material names to Tidy3DMedium instances. Defaults to material_name_to_medium.
extend_ports: The extension length for ports.
port_offset: The offset for ports. Defaults to 0.2.
pad_xy_inner: The inner padding in the xy-plane. Defaults to 2.0.
pad_xy_outer: The outer padding in the xy-plane. Defaults to 2.0.
pad_z_inner: The inner padding in the z-direction. Defaults to 0.0.
pad_z_outer: The outer padding in the z-direction. Defaults to 0.0.
dilation: Dilation of the polygon in the base by shifting each edge along its normal outwards direction by a distance;
wavelength: The wavelength for the ComponentModeler. Defaults to 1.55.
bandwidth: The bandwidth for the ComponentModeler. Defaults to 0.2.
num_freqs: The number of frequencies for the ComponentModeler. Defaults to 21.
min_steps_per_wvl: The minimum number of steps per wavelength for the ComponentModeler. Defaults to 30.
center_z: The z-coordinate for the center of the ComponentModeler.
If None, the z-coordinate of the component is used. Defaults to None.
sim_size_z: simulation size um in the z-direction for the ComponentModeler. Defaults to 4.
port_size_mult: The size multiplier for the ports in the ComponentModeler. Defaults to (4.0, 3.0).
run_only: The run only specification for the ComponentModeler. Defaults to None.
element_mappings: The element mappings for the ComponentModeler. Defaults to ().
extra_monitors: The extra monitors for the ComponentModeler. Defaults to None.
mode_spec: The mode specification for the ComponentModeler. Defaults to td.ModeSpec(num_modes=1, filter_pol="te").
boundary_spec: The boundary specification for the ComponentModeler.
Defaults to td.BoundarySpec.all_sides(boundary=td.PML()).
symmetry (tuple[Symmetry, Symmetry, Symmetry], optional): The symmetry for the simulation. Defaults to (0,0,0).
run_time: The run time for the ComponentModeler.
shutoff: The shutoff value for the ComponentModeler. Defaults to 1e-5.
folder_name: The folder name for the ComponentModeler in flexcompute website. Defaults to "default".
dirpath: Optional directory path for writing the Sparameters. Defaults to "~/.gdsfactory/sparameters".
verbose: Whether to print verbose output for the ComponentModeler. Defaults to True.
plot_simulation_layer_name: Optional layer name to plot. Defaults to None.
plot_simulation_port_index: which port index to plot. Defaults to 0.
plot_simulation_z: which z coordinate to plot. Defaults to None.
plot_simulation_x: which x coordinate to plot. Defaults to None.
plot_mode_index: which mode index to plot. Defaults to 0.
plot_mode_port_name: which port name to plot. Defaults to None.
plot_epsilon: whether to plot epsilon. Defaults to False.
filepath: Optional file path for the S-parameters. If None, uses hash of simulation.
overwrite: Whether to overwrite existing S-parameters. Defaults to False.
kwargs: Additional keyword arguments for the tidy3d Simulation constructor.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| overwrite: bool = False, | ||
| upload_only: bool = False, | ||
| **kwargs: Any, | ||
| ) -> Sparameters: |
There was a problem hiding this comment.
The function can return None when the simulation status is not "completed" (due to the inverted condition on line 613). The return type annotation specifies Sparameters, which does not include None as a valid return type. This creates a type safety issue. The return type should be updated to Sparameters | None or the logic should ensure a valid return in all paths.
Suggested change
| ) -> Sparameters: | |
| ) -> Sparameters | None: |
gplugins/tidy3d/component.py
Outdated
| 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),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.
The freqs parameter for FieldMonitor expects a list or array of frequencies, but td.constants.C_0 / wavelength produces a scalar value. This should be wrapped in a list or array, for example: freqs=[td.constants.C_0 / wavelength].
Suggested change
| 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)) | |
| 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)) |
gplugins/tidy3d/component.py
Outdated
| 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] | ||
|
|
There was a problem hiding this comment.
Hardcoded layer name "core" may not exist in all component configurations. The code should either validate that the "core" layer exists before accessing it, use a configurable layer name, or handle the potential KeyError/AttributeError that could occur if this layer is missing.
Suggested change
| if "core" not in c.layers: | |
| raise ValueError('Layer "core" not found in component layers. Please ensure the component has a "core" layer.') |
gplugins/tidy3d/component.py
Outdated
| 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),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.
Missing space after comma in the interval_space parameter. The code has interval_space=(4, 4, 1),freqs= but should have a space between the comma and freqs.
Suggested change
| 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)) | |
| 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)) |
| return s | ||
| if not upload_only: | ||
| s = modeler.run() | ||
| if s.status != "completed": |
There was a problem hiding this comment.
The logic condition if s.status != "completed" appears inverted. When the status is NOT completed (e.g., failed, pending, etc.), the code proceeds to extract S-parameters and save results. This should only happen when the status IS "completed". The condition should be if s.status == "completed" instead.
Suggested change
| if s.status != "completed": | |
| if s.status == "completed": |
Comment on lines
611
to
631
| if not upload_only: | ||
| s = modeler.run() | ||
| if s.status != "completed": | ||
| for port_in in s.port_in.values: | ||
| for port_out in s.port_out.values: | ||
| for mode_index_in in s.mode_index_in.values: | ||
| for mode_index_out in s.mode_index_out.values: | ||
| sp[f"{port_in}@{mode_index_in},{port_out}@{mode_index_out}"] = ( | ||
| s.sel( | ||
| port_in=port_in, | ||
| port_out=port_out, | ||
| mode_index_in=mode_index_in, | ||
| mode_index_out=mode_index_out, | ||
| ).values | ||
| ) | ||
|
|
||
| frequency = s.f.values | ||
| sp["wavelengths"] = td.constants.C_0 / frequency | ||
| np.savez_compressed(filepath, **sp) | ||
| print(f"Simulation saved to {filepath!r}") | ||
| return sp |
There was a problem hiding this comment.
The redundant condition check if not upload_only is unnecessary since the code is already within an else block where upload_only is False (from line 598's if upload_only). This condition will always be True and can be removed.
Suggested change
| if not upload_only: | |
| s = modeler.run() | |
| if s.status != "completed": | |
| for port_in in s.port_in.values: | |
| for port_out in s.port_out.values: | |
| for mode_index_in in s.mode_index_in.values: | |
| for mode_index_out in s.mode_index_out.values: | |
| sp[f"{port_in}@{mode_index_in},{port_out}@{mode_index_out}"] = ( | |
| s.sel( | |
| port_in=port_in, | |
| port_out=port_out, | |
| mode_index_in=mode_index_in, | |
| mode_index_out=mode_index_out, | |
| ).values | |
| ) | |
| frequency = s.f.values | |
| sp["wavelengths"] = td.constants.C_0 / frequency | |
| np.savez_compressed(filepath, **sp) | |
| print(f"Simulation saved to {filepath!r}") | |
| return sp | |
| s = modeler.run() | |
| if s.status != "completed": | |
| for port_in in s.port_in.values: | |
| for port_out in s.port_out.values: | |
| for mode_index_in in s.mode_index_in.values: | |
| for mode_index_out in s.mode_index_out.values: | |
| sp[f"{port_in}@{mode_index_in},{port_out}@{mode_index_out}"] = ( | |
| s.sel( | |
| port_in=port_in, | |
| port_out=port_out, | |
| mode_index_in=mode_index_in, | |
| mode_index_out=mode_index_out, | |
| ).values | |
| ) | |
| frequency = s.f.values | |
| sp["wavelengths"] = td.constants.C_0 / frequency | |
| np.savez_compressed(filepath, **sp) | |
| print(f"Simulation saved to {filepath!r}") | |
| return sp |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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
Change write_sparameters to upload a visualization-focused simulation to Tidy3D without running it, returning the uploaded task handle instead of local S-parameter results.
New Features:
Enhancements: