-
Notifications
You must be signed in to change notification settings - Fork 65
Add interpolation to EME #2973
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.
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
base: develop
Are you sure you want to change the base?
Add interpolation to EME #2973
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,9 @@ | |
| from tidy3d.components.base import Tidy3dBaseModel, skip_if_fields_missing | ||
| from tidy3d.components.geometry.base import Box | ||
| from tidy3d.components.grid.grid import Coords1D | ||
| from tidy3d.components.mode_spec import ModeSpec | ||
| from tidy3d.components.mode_spec import ModeInterpSpec, ModeSpec | ||
| from tidy3d.components.structure import Structure | ||
| from tidy3d.components.types import ArrayFloat1D, Axis, Coordinate, Size, TrackFreq | ||
| from tidy3d.components.types import ArrayFloat1D, Axis, Coordinate, Size | ||
| from tidy3d.constants import RADIAN, fp_eps, inf | ||
| from tidy3d.exceptions import SetupError, ValidationError | ||
|
|
||
|
|
@@ -26,13 +26,14 @@ | |
| class EMEModeSpec(ModeSpec): | ||
| """Mode spec for EME cells. Overrides some of the defaults and allowed values.""" | ||
|
|
||
| track_freq: Union[TrackFreq, None] = pd.Field( | ||
| None, | ||
| title="Mode Tracking Frequency", | ||
| description="Parameter that turns on/off mode tracking based on their similarity. " | ||
| "Can take values ``'lowest'``, ``'central'``, or ``'highest'``, which correspond to " | ||
| "mode tracking based on the lowest, central, or highest frequency. " | ||
| "If ``None`` no mode tracking is performed, which is the default for best performance.", | ||
| interp_spec: Optional[ModeInterpSpec] = pd.Field( | ||
| ModeInterpSpec.cheb(num_points=3, reduce_data=True), | ||
| title="Mode frequency interpolation specification", | ||
| description="Specification for computing modes at a reduced set of frequencies and " | ||
| "interpolating to obtain results at all requested frequencies. This can significantly " | ||
| "reduce computational cost for broadband simulations where modes vary smoothly with " | ||
| "frequency. Requires frequency tracking to be enabled (``sort_spec.track_freq`` must " | ||
| "not be ``None``) to ensure consistent mode ordering across frequencies.", | ||
| ) | ||
|
Comment on lines
+29
to
37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The description states "Requires frequency tracking to be enabled ( Prompt To Fix With AIThis is a comment left during a code review.
Path: tidy3d/components/eme/grid.py
Line: 29:37
Comment:
**logic:** The description states "Requires frequency tracking to be enabled (`sort_spec.track_freq` must not be `None`)", but `EMEModeSpec` no longer has a `track_freq` field (it was removed in this PR). The docstring should be updated to reflect the actual requirements. Consider checking the parent `ModeSpec` class's `sort_spec` field instead if tracking is needed.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| angle_theta: Literal[0.0] = pd.Field( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,7 @@ | |
| from tidy3d.components.types.mode_spec import ModeSpecType | ||
| from tidy3d.components.types.monitor_data import ModeSolverDataType | ||
| from tidy3d.components.validators import ( | ||
| _warn_interp_num_points, | ||
| validate_freqs_min, | ||
| validate_freqs_not_empty, | ||
| ) | ||
|
|
@@ -515,8 +515,8 @@ | |
| A mode solver data type object containing the effective index and mode fields. | ||
| """ | ||
|
|
||
| if self.mode_spec.interp_spec is not None: | ||
| _warn_interp_num_points(self.mode_spec.interp_spec, self.freqs) | ||
| # if self.mode_spec.interp_spec is not None: | ||
| # _warn_interp_num_points(self.mode_spec.interp_spec, self.freqs) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbochkov-flexcompute Do we need these warnings? They are annoying in an EME simulation where interpolation is the default, and falling back to the actual frequencies specified is even better (not worth warning about necessarily)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think I was overcautious here. I think it was making more sense when interp_spec was outside of mode_spec, because to enable that the user had to make a conscious effort of specifying interp_spec in a monitor and it felt like we should let them know it was for nothing lol
Comment on lines
+518
to
+519
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: validation commented out. Add a comment explaining why this warning was disabled. Prompt To Fix With AIThis is a comment left during a code review.
Path: tidy3d/components/mode/mode_solver.py
Line: 518:519
Comment:
**style:** validation commented out. Add a comment explaining why this warning was disabled.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| if self.mode_spec.angle_rotation and np.abs(self.mode_spec.angle_theta) > 0: | ||
| return self.rotated_mode_solver_data | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -433,7 +433,7 @@ def sampling_points(self, freqs: FreqArray) -> FreqArray: | |
| >>> interp_spec = ModeInterpSpec.cheb(num_points=10) | ||
| >>> sampling_freqs = interp_spec.sampling_points(freqs) | ||
| """ | ||
| if self.num_points > len(freqs): | ||
| if self.num_points >= len(freqs): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbochkov-flexcompute not really needed for EME, but should we make this change?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's more accurate, thanks for catching that |
||
| return freqs | ||
|
Comment on lines
+436
to
437
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: changed from Prompt To Fix With AIThis is a comment left during a code review.
Path: tidy3d/components/mode_spec.py
Line: 436:437
Comment:
**style:** changed from `>` to `>=` but now the validation warning is commented out in monitor.py:440 and mode_solver.py:518-519. Users won't be warned when `num_points >= len(freqs)`, which defeats the purpose of this optimization. Either restore the warning or add a comment explaining why it's acceptable.
How can I resolve this? If you propose a fix, please make it concise. |
||
| return self.sampling_spec.sampling_points(freqs) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,7 @@ | |
| assert_plane, | ||
| validate_freqs_min, | ||
| validate_freqs_not_empty, | ||
| validate_interp_num_points, | ||
| ) | ||
| from .viz import ARROW_ALPHA, ARROW_COLOR_MONITOR | ||
|
|
||
|
|
@@ -429,15 +429,22 @@ | |
| ) | ||
| return val | ||
|
|
||
| @property | ||
| def _stored_freqs(self) -> list[float]: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbochkov-flexcompute this is to update
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, yeah, I updated |
||
| """Return actually stored frequencies of the data.""" | ||
| return self.mode_spec._sampling_freqs_mode_solver_data(freqs=self.freqs) | ||
|
|
||
| def _storage_size_solver(self, num_cells: int, tmesh: ArrayFloat1D) -> int: | ||
| """Size of intermediate data recorded by the monitor during a solver run.""" | ||
| # Need to store all fields on the mode surface | ||
| bytes_single = BYTES_COMPLEX * num_cells * len(self.freqs) * self.mode_spec.num_modes * 6 | ||
| bytes_single = ( | ||
| BYTES_COMPLEX * num_cells * len(self._stored_freqs) * self.mode_spec.num_modes * 6 | ||
| ) | ||
| if self.mode_spec.precision == "double": | ||
| return 2 * bytes_single | ||
| return bytes_single | ||
|
|
||
| _warn_interp_num_points = validate_interp_num_points() | ||
| # _warn_interp_num_points = validate_interp_num_points() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: validation commented out instead of removed. Add a comment explaining why (e.g., "Warning disabled because EME now defaults to interpolation" or similar context). Prompt To Fix With AIThis is a comment left during a code review.
Path: tidy3d/components/monitor.py
Line: 440:440
Comment:
**style:** validation commented out instead of removed. Add a comment explaining why (e.g., "Warning disabled because EME now defaults to interpolation" or similar context).
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
|
|
||
| class FieldMonitor(AbstractFieldMonitor, FreqMonitor): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@momchil-flex We could change this to be GVD based (i.e. polynomial with three sampling points concentrated around the central frequency). But I'm not sure that's better in general. It can lower the accuracy towards the edge of the spectrum, and I haven't had a problem with mode tracking. And the user can customize this if needed