feat: surface monitors (FXC-1635) #2891
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (3)
-
tidy3d/components/tcad/data/monitor_data/abstract.py, line 63 (link)logic: Method
_symmetry_expanded_copy_basewas removed but is still being called here. This will cause an AttributeError at runtime. -
tidy3d/components/data/unstructured/base.py, line 235-237 (link)logic: Return type should be
UnstructuredDatasetto match the new class hierarchy -
tidy3d/components/data/unstructured/base.py, line 641 (link)syntax: Return type annotation should be
UnstructuredDatasetto match the method implementation
28 files reviewed, 16 comments
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/data/dataset.pyLines 516-524 Lines 553-561 tidy3d/components/data/monitor_data.pyLines 1565-1576 tidy3d/components/data/unstructured/base.pyLines 375-383 Lines 921-929 Lines 964-972 Lines 970-978 Lines 1176-1184 Lines 1242-1250 tidy3d/components/monitor.pyLines 1649-1672 Lines 1670-1686 Lines 1716-1758 tidy3d/components/simulation.pyLines 3967-3988 tidy3d/components/viz/axes_utils.pyLines 151-161 Lines 206-214 tidy3d/packaging.pyLines 171-180 |
dbochkov-flexcompute
requested review from
marc-flex,
momchil-flex and
yaugenst-flex
marc-flex
left a comment
marc-flex
left a comment
There was a problem hiding this comment.
Thanks @dbochkov-flexcompute! Great refactor of the unstructured grid and the data array tests!
Left some minor comments but overall looks good to me
tests/test_data/test_data_arrays.py
Outdated
| # SURFACE_FIELD_MONITOR, | ||
| # SURFACE_FIELD_TIME_MONITOR, |
There was a problem hiding this comment.
We can remove this, right?
tests/test_data/test_monitor_data.py
Outdated
| assert np.all(np.isreal(intensity.values.values)) | ||
|
|
||
|
|
||
| def test_surface_field_time_data(): |
There was a problem hiding this comment.
This test and the one above are very similar. I wonder if they could be combined using @pytest.mark.parametrize().
I guess some of the duplicated asserts would be averted though maybe is less clear/explicit? What do you think?
The following two I think could benefit from combining them and have 2 instead of 4 tests?
yaugenst-flex
left a comment
yaugenst-flex
left a comment
There was a problem hiding this comment.
Big effort! Looks great overall just a couple of comments.
| grid_spec=td.GridSpec.auto(wavelength=1), | ||
| ) | ||
|
|
||
| # monitor must be volumetric |
There was a problem hiding this comment.
Wondering about 2d simulations - does the surface monitor work for those?
There was a problem hiding this comment.
currently, not, which reminds me to add validator for that. I should be able to add this option later either in this PR or in a follow-up PR. To keep it simple, I would still probably return a 3d surface (ribbon) with a finite width in the zero size dimension instead of a 2d curve
| @classmethod | ||
| def _point_dims(cls) -> pd.PositiveInt: | ||
| """Dimensionality of stored surface point coordinates.""" | ||
| return 3 | ||
|
|
||
| @classmethod | ||
| def _cell_num_vertices(cls) -> pd.PositiveInt: | ||
| """Number of vertices in a cell.""" | ||
| return 3 |
There was a problem hiding this comment.
Wondering whether these should rather be properties?
There was a problem hiding this comment.
we need access to these in validators and in class method from_vtu, and it seems like @classmethod + @Property is depreciated in python 11 python/cpython#119628
tidy3d/components/viz/axes_utils.py
Outdated
| def _plot(*args, **kwargs): | ||
| """New plot function using a generated plotter if None.""" | ||
| # Extract decorator-specific parameters | ||
| plotter = kwargs.get("plotter") |
There was a problem hiding this comment.
plotter is not required to be a keyword argument as per the definition of plot(), so if you were to pass a plotter positionally, then the decorator here would still inject a keyword copy and you'd end up with an exception. This would be more robustly handled via inspect.signature().
| sel_kwargs_only_lists = { | ||
| key: value if isinstance(value, list) else [value] for key, value in sel_kwargs.items() | ||
| } |
There was a problem hiding this comment.
Since this converts all kwargs to lists, wouldn't this end up breaking "non-indexing" arguments? .sel has a couple of those: https://docs.xarray.dev/en/latest/generated/xarray.DataArray.sel.html#xarray.DataArray.sel
Or do we not care since this is a private method and we know we're not going to use those arguments?
| if all(sym == 0 for sym in self.symmetry): | ||
| return data | ||
|
|
||
| new_data = copy.copy(data) |
There was a problem hiding this comment.
I guess this is safe but it doesn't look to me like the copy is necessary here? new_data is never mutated.
| def _is_notebook() -> bool: | ||
| """Detect if running in Jupyter notebook. | ||
|
|
||
| Returns | ||
| ------- | ||
| bool | ||
| True if running in Jupyter notebook, False otherwise. | ||
| """ | ||
| try: | ||
| from IPython import get_ipython | ||
|
|
||
| ipython = get_ipython() | ||
| if ipython is None: | ||
| return False | ||
| return "IPKernelApp" in ipython.config | ||
| except (ImportError, AttributeError): | ||
| return False |
There was a problem hiding this comment.
I haven't played around with this much but I'm wondering whether the approach is robust against different notebook environments like JupyterLab, VSCode, Colab? It might be more robust to check the class name of the shell directly, i.e., based on get_ipython().__class__.__name__. Would need to test though.
There was a problem hiding this comment.
expanded this function using recommendations from the web and tested in jupyter-lab, vscode, and colab - all seem to work alright
| colocate: Literal[False] = pydantic.Field( | ||
| False, | ||
| title="Colocate Fields", | ||
| description="For surface monitors fields are always colocated on surface.", |
There was a problem hiding this comment.
if they are always colocated on the surface, would colocate always be True instead of False?
There was a problem hiding this comment.
yeah, it's a bit confusing here for some internal reasons. But I will change it to True
tidy3d/components/medium.py
Outdated
| return Lorentz.from_nk(n, k, freq, **kwargs) | ||
|
|
||
|
|
||
| def _is_pec_like(medium: MediumType3D) -> bool: |
There was a problem hiding this comment.
is there a reason to have this be a separate function instead of a property of the mediums? I.e. - medium.is_pec_like?
There was a problem hiding this comment.
no reason, implemented now
weiliangjin2021
left a comment
weiliangjin2021
left a comment
There was a problem hiding this comment.
Played a bit with pyvista based on your unit tests. Looks quite powerful!
| return val | ||
|
|
||
| @cached_property | ||
| def is_pec_like(self): |
There was a problem hiding this comment.
what about is_good_conductor to be more precise?
| return {field_name: field for field_name, field in fields.items() if field is not None} | ||
|
|
||
| @property | ||
| def intensity(self) -> TriangularSurfaceDataset: |
There was a problem hiding this comment.
here intensity refers to electric field intensity. should we be concerned if someone confuses it with E, or H, or summed field intensity.
| if self.H is not None: | ||
| # we assume that if data is None it means field is zero on that side (e.g. PEC) | ||
| H_inside = ( | ||
| self.H.sel(side="inside", drop=True) if "inside" in self.H.values.side else None |
There was a problem hiding this comment.
Somehow I couldn't find where side is defined in the dataset.
There was a problem hiding this comment.
it's defined in data arrays https://github.com/flexcompute/tidy3d/pull/2891/files#diff-834da9ec0c6f019fc8dde7c9b3cd1d2eafb780dadfadb21e94c0a313be526076 similar to mode_index, direction, etc
a few examples (unfortunately, need to re-ran to see visualization):
Greptile Overview
Updated On: 2025-10-14 22:39:42 UTC
Greptile Summary
This PR introduces comprehensive surface monitoring capabilities to Tidy3D, allowing users to monitor electromagnetic fields directly on Perfect Electric Conductor (PEC) and lossy metal surfaces. The implementation adds two new monitor types:
SurfaceFieldMonitorfor frequency-domain monitoring andSurfaceFieldTimeMonitorfor time-domain monitoring. These monitors capture electric and magnetic fields on triangular surface meshes rather than regular Cartesian grids.The core functionality includes new type definitions (
EMSurfaceField), unstructured dataset support (TriangularSurfaceDataset), and corresponding data classes (SurfaceFieldData,SurfaceFieldTimeData) that handle field storage and manipulation on surface meshes. The monitors integrate with the existing simulation validation framework to ensure they only operate where PEC or lossy metal structures exist. PyVista support has been added as an optional dependency for 3D surface visualization, following the established pattern of optional visualization libraries.The implementation extends the existing monitor infrastructure while maintaining backward compatibility, adding specialized handling for surface current density calculations through cross products of magnetic field discontinuities across conductor boundaries. All new functionality includes comprehensive test coverage and follows the established code patterns for monitor validation, data handling, and visualization.
Important Files Changed
Changed Files
Confidence score: 3/5
AbstractTcadFieldData.symmetry_expanded_copy(line 63) calling removed_symmetry_expanded_copy_basemethod, type annotation inconsistencies in base classes, and complex class hierarchy changes that could affect existing functionalitytidy3d/components/tcad/data/monitor_data/abstract.pywhich has a critical broken method call, and the unstructured data base class refactoring which may impact inheritance behaviorContext used:
dashboard- When modifying a piece of logic, ensure the change is propagated to all independent functions or cal... (source)