Skip to content

Upload, but not run, simulation for write_sparameters routine #728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jackgdsf wants to merge 5 commits into
base: main
Choose a base branch
from
+57 −27
Open

Upload, but not run, simulation for write_sparameters routine #728

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f44bdf3
in the write_sparameters method we only allow upload and not run, all…
heydaridlm Dec 8, 2025
c2134d2
flag for upload only prevents breaking the function for other users
heydaridlm Dec 11, 2025
f206f13
tidy.constants.C_0 for speed of light
heydaridlm Dec 11, 2025
8e7fff3
fixed precom errors
heydaridlm Dec 16, 2025
4ff59e1
Merge branch 'main' into sim_upload_only
Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 57 additions & 27 deletions gplugins/tidy3d/component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
import numpy as np
import tidy3d as td
import tidy3d.web as web
import tidy3d.web.api.webapi as webapi
from gdsfactory.component import Component
from gdsfactory.pdk import get_layer_stack
from gdsfactory.technology import LayerStack
from pydantic import NonNegativeFloat
from tidy3d.components.geometry.base import from_shapely
from tidy3d.components.monitor import FieldMonitor
from tidy3d.components.types import Symmetry
from tidy3d.plugins.smatrix import ModalComponentModeler, Port

Expand Down Expand Up @@ -272,7 +274,7 @@ def get_component_modeler(

cz = np.round(cz, abs(int(np.log10(grid_eps)))).item()

freqs = td.C_0 / np.linspace(
freqs = td.constants.C_0 / np.linspace(
wavelength - bandwidth / 2, wavelength + bandwidth / 2, num_freqs
)

Expand Down Expand Up @@ -437,6 +439,7 @@ def write_sparameters(
plot_epsilon: bool = False,
filepath: PathType | None = None,
overwrite: bool = False,
upload_only: bool = False,
**kwargs: Any,
) -> Sparameters:

@gemini-code-assist gemini-code-assist Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
) -> Sparameters:
) -> Sparameters | str:

"""Writes the S-parameters for a component.
Comment on lines +442 to 445

@sourcery-ai sourcery-ai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down Expand Up @@ -482,7 +485,7 @@ def write_sparameters(
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.

upload_only: Whether to upload the simulation to the cloud only. Defaults to False.
"""
layer_stack = layer_stack or get_layer_stack()

Expand Down Expand Up @@ -581,31 +584,58 @@ def write_sparameters(
return dict(np.load(filepath))
else:
time.sleep(0.2)
modeler_data = web.run(
modeler, # TODO: web.run does not currently support ModalComponentModeler, need to convert to tidy3d_stub.SimulationType
task_name=task_name,
verbose=verbose,
path=path_dir / "simulation.hdf5",
)
s = modeler_data.smatrix()
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
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),
Comment on lines +587 to +596

@sourcery-ai sourcery-ai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

freqs=td.constants.C_0 / wavelength,
center=(c.center[0], c.center[1], cz),
size=(c.size[0], c.size[1], 0),
)
Comment on lines +593 to +600

@gemini-code-assist gemini-code-assist Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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),
)


sim_with_sources = modeler.simulation.copy(
update={
"sources": plot_sources,
"monitors": list(modeler.simulation.monitors)
+ plot_monitors
+ [birdseye],
}
)

s = webapi.upload(sim_with_sources, task_name=folder_name)

@gemini-code-assist gemini-code-assist Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
s = webapi.upload(sim_with_sources, task_name=folder_name)
s = webapi.upload(sim_with_sources, task_name=task_name, folder_name=folder_name)

return s
else:
modeler_data = web.run(
modeler, # TODO: web.run does not currently support ModalComponentModeler, need to convert to tidy3d_stub.SimulationType
task_name=task_name,
verbose=verbose,
path=path_dir / "simulation.hdf5",
)
s = modeler_data.smatrix()
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


def write_sparameters_batch(
Expand Down
Loading