Skip to content

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

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
heydaridlm wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Changes from 3 commits
Commits
Show all changes
4 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
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
59 changes: 39 additions & 20 deletions gplugins/tidy3d/component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
from functools import cached_property
from typing import Any, Literal

import tidy3d.web.api.webapi as web
from tidy3d.components.monitor import FieldMonitor

import matplotlib.pyplot as plt
import numpy as np
import tidy3d as td
Expand Down Expand Up @@ -277,7 +280,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 @@ -445,6 +448,7 @@ def write_sparameters(
plot_epsilon: bool = False,
filepath: PathType | None = None,
overwrite: bool = False,
upload_only: bool = False,
**kwargs: Any,
) -> Sparameters:
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

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:

Copilot uses AI. Check for mistakes.
"""Writes the S-parameters for a component.
Expand Down Expand Up @@ -591,25 +595,40 @@ def write_sparameters(
return dict(np.load(filepath))
else:
time.sleep(0.2)
s = modeler.run()
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]

Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

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.')

Copilot uses AI. Check for mistakes.
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))
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

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))

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

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))

Copilot uses AI. Check for mistakes.

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

s = web.upload(sim_with_sources, task_name=folder_name)
return s
if not upload_only:
s = modeler.run()
if s.status != "completed":
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

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":

Copilot uses AI. Check for mistakes.
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
Comment on lines 623 to 643
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

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

Copilot uses AI. Check for mistakes.


def write_sparameters_batch(
Expand Down
Loading