Momchil/sym data #297
Momchil/sym data #297
Conversation
There was a problem hiding this comment.
looks good, a few things
- be sure to put class links in the docstrings where appropriate. also note that the docs probably won't render the types of things like
AxisSymmetry. - A few more comments won't hurt anyone, especially explaining the
_sym_dictusage - Consider refactoring getitem in
SimulationData.
After these, feel free to merge.
tidy3d/components/data.py
Outdated
| # Apply the correct +/-1 for the data_key component | ||
| new_data[{dim_name: flip_inds}] *= sym * self._sym_dict[data_key][dim] | ||
| sym_eval = self._sym_dict.get(data_key) | ||
| if sym_eval: |
There was a problem hiding this comment.
if sym_eval is not None is recommended here to be explicit. For example, what if it's 0?
There was a problem hiding this comment.
Technically it should be a list, but I agree.
tidy3d/components/data.py
Outdated
| """Create a new AbstractFieldData subclass by interpolating on the supplied ``new_grid``, | ||
| using symmetries as defined by ``sym_center`` and ``symmetry``.""" | ||
| def apply_syms(self, grid_dict: Dict[str, Coords], sym_center: Coordinate, symmetry: Symmetry): | ||
| """Create a new SpatialCollectionData subclass by interpolating on the supplied |
tidy3d/components/data.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| grid_dict : Dict[str, Coords] |
tidy3d/components/data.py
Outdated
| Parameters | ||
| ---------- | ||
| grid_dict : Dict[str, Coords] | ||
| Mapping of the data labels in the SpatialCollectionData to new coordinates on which |
There was a problem hiding this comment.
just apply where appropriate in docstring.
tidy3d/components/data.py
Outdated
| zipped = zip("xyz", sym_center, symmetry) | ||
| for dim, (dim_name, center, sym) in enumerate(zipped): | ||
| # Continue if no symmetry or the data key is not in the supplied grid_dict | ||
| if sym == 0 or not grid_dict.get(data_key): |
There was a problem hiding this comment.
better to use grid_dict.get(data_key) is not None instead of not grid_dict.get(data_key).
tidy3d/plugins/mode/mode_solver.py
Outdated
| @@ -349,7 +349,7 @@ def solve(self) -> ModeSolverData: | |||
| ) | |||
|
|
|||
| field_data = ModeFieldData(data_dict=data_dict).apply_syms( | |||
| plane_grid, self.simulation.center, self.simulation.symmetry | |||
| plane_grid.yee.grid_dict, self.simulation.center, self.simulation.symmetry | |||
| ... name='eps_monitor') | ||
| """ | ||
|
|
||
| _data_type: Literal["PermittivityData"] = pydantic.Field("PermittivityData") |
There was a problem hiding this comment.
dont think we need these anymore? did we figure that out?
There was a problem hiding this comment.
Still need them on the backend. I can show you where if you want to think about how to eliminate them.
tidy3d/components/grid.py
Outdated
| ---------- | ||
| axis : Axis | ||
| Axis along which to pick the subspace. | ||
| ind_beg : int, optional |
There was a problem hiding this comment.
int = 0 instead of Optional
tidy3d/components/grid.py
Outdated
| Parameters | ||
| ---------- | ||
| axis : Axis | ||
| Axis along which to pick the subspace. |
There was a problem hiding this comment.
maybe instead of Axis, say Axis index (0,1,2) along which... so people know it's an int
| @@ -1018,6 +1133,26 @@ def __getitem__(self, monitor_name: str) -> Union[Tidy3dDataArray, xr.Dataset]: | |||
| monitor_data = self.monitor_data.get(monitor_name) | |||
| if isinstance(monitor_data, MonitorData): | |||
| return monitor_data.data | |||
| if isinstance(monitor_data, SpatialCollectionData): | |||
There was a problem hiding this comment.
This chunk probably needs a bit more documentation at the least. Alternatively, we could create methods / properties in MonitorData, SpatialCollectionData, PermittivityData that define what gets returned when an object of this type is selected by SimulationData. For example
class MonitorData(...):
# some stuff
@property
sim_data_getitem(self) -> xr.DataArray:
"""What gets returned by sim_data['monitor_data_name']"""
return self.dataand then in SimulationData.__get_item__:
def __getitem__(self, name):
monitor_data = self.monitor_data.get(monitor_name)
if monitor_data is None:
raise Tidy3dKeyError("Monitor '{monitor_name}' not found in SimulationData.")
return monitor_data.sim_data_getitmwhat do you think? Would just require putting each of these is instance blocks in their respective classes.
field_data = sim_data[monitor_name]