Skip to content
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

Jump to bottom

Use default td.ModeSpec in mode monitors #2058

Open
wants to merge 2 commits into
base: pre/2.8
Choose a base branch
from
Open

Use default td.ModeSpec in mode monitors #2058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb3debd
Use default `td.ModeSpec` in mode monitors
yaugenst-flex Nov 6, 2024
1eafd9d
Docstring support for `default_factory`
yaugenst-flex Nov 11, 2024
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Enabled the user to set the `ModeMonitor.colocate` field and changed to `True` by default (fields were actually already returned colocated even though this field was `False` previously).
- More robust mode solver at radio frequencies.
- Disallow very small polygons when subdividing 2D structures.
- `CustomMedium` design regions require far less data when performing inverse design by reducing adjoint field monitor size for dims with one pixel.
- Calling `.values` on `DataArray` no longer raises a `DeprecationWarning` during automatic differentiation.
- `ModeMonitor` and `ModeSolverMonitor` now use the default `td.ModeSpec()` when `mode_spec` is not provided.

### Fixed
- Significant speedup for field projection computations.
Expand Down
13 changes: 6 additions & 7 deletions tests/test_components/test_monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,16 +337,16 @@ def test_monitor():
m5 = td.ModeMonitor(
size=(1, 1, 0), center=center, mode_spec=td.ModeSpec(), freqs=FREQS, name="test_mon"
)
m6 = td.ModeSolverMonitor(
m6 = td.ModeMonitor(size=(1, 1, 0), center=center, freqs=FREQS, name="test_mon")
m7 = td.ModeSolverMonitor(
size=(1, 1, 0),
center=center,
mode_spec=td.ModeSpec(),
freqs=FREQS,
name="test_mon",
direction="-",
)
m7 = td.PermittivityMonitor(size=size, center=center, freqs=FREQS, name="perm")
m8 = td.DirectivityMonitor(
m8 = td.PermittivityMonitor(size=size, center=center, freqs=FREQS, name="perm")
m9 = td.DirectivityMonitor(
size=size,
center=center,
theta=thetas,
Expand All @@ -355,12 +355,11 @@ def test_monitor():
freqs=FREQS,
name="directivity",
)
m10 = td.PermittivityMonitor(size=size, center=center, freqs=FREQS, name="perm")

tmesh = np.linspace(0, 1, 10)

for m in [m1, m2, m3, m4, m5, m6, m7, m8]:
# m.plot(y=2)
# plt.close()
for m in [m1, m2, m3, m4, m5, m6, m7, m8, m9, m10]:
m.storage_size(num_cells=100, tmesh=tmesh)

for m in [m2, m4]:
Expand Down
7 changes: 5 additions & 2 deletions tidy3d/components/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,8 +1092,12 @@ def generate_docstring(cls) -> str:
data_type = field._type_display()

# get default values
field_info = field.field_info
default_val = field.get_default()
if "=" in str(default_val):
if field_info.default_factory is not None:
# handle default_factory
default_val = f"{field_info.default_factory.__name__}()"
elif "=" in str(default_val):
# handle cases where default values are pydantic models
default_val = f"{default_val.__class__.__name__}({default_val})"
default_val = (", ").join(default_val.split(" "))
Expand All @@ -1103,7 +1107,6 @@ def generate_docstring(cls) -> str:
doc += f" {field_name} : {data_type}{default_str}\n"

# get field metadata
field_info = field.field_info
doc += " "

# add units (if present)
Expand Down
2 changes: 1 addition & 1 deletion tidy3d/components/monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ class AbstractModeMonitor(PlanarMonitor, FreqMonitor):
""":class:`Monitor` that records mode-related data."""

mode_spec: ModeSpec = pydantic.Field(
...,
default_factory=ModeSpec,
Copy link
Collaborator

@tylerflex tylerflex Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

generally we just do ModeSpec() (no default factory). not sure if there's a substantial difference here, but i just dont know how the docs will handle default factories

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change it or do we want to check how the docs look? Generally I would've thought that it's best to instantiate defaults (if they are classes) only on instantiation of the class, otherwise 1. you get object creation on import, which slows imports down and 2. it leads to shared state between different mode monitor instances. All new mode monitors will share the same default instance of mode_spec, and if you happen to make a change that modifies this default instance (let's say you write something into attrs), then this change will affect all other mode monitors too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess we could see how the docs looks, just might require some modification of this method that sets the docstring, we'd need to add handling for defaultfactory. Also cc'ing @daquinteroflex just so he knows since he's working on a sphinx plugin related to this.

Copy link
Contributor Author

@yaugenst-flex yaugenst-flex Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I took a stab at this in 1eafd9d which I think would correctly handle cases where we just use a class as a default factory. It wouldn't handle lambdas nicely, although if necessary I can also add support for that (by just printing the lambda definition).

title="Mode Specification",
description="Parameters to feed to mode solver which determine modes measured by monitor.",
)
Expand Down
Loading