Vasily/bandgap_monitor_update #2344
Conversation
| field_data = {field: values.get(field) for field in ["Ec", "Ev", "Ei", "Efn", "Efp"]} | ||
|
|
||
| for field, data in field_data.items(): |
There was a problem hiding this comment.
This field_data seems redundant? The for-loop could be over the fields, i.e., for field in ["Ec", "Ev", "Ei", "Efn", "Efp"]: as you're doing below.
Not particularly bother by it though
There was a problem hiding this comment.
Here I have the loop where I need co sweep "data" and "data" variables together. I thought that this way is better to preserve that I they are updated each iteration of the loop.
| if not isinstance(self.Ec, TriangularGridDataset): | ||
| raise DataError( | ||
| "Bandgap monitor slice plot can be done only for a 2D unstructured dataset." | ||
| ) |
There was a problem hiding this comment.
Do we really need this restriction? If we had a 3D monitor we would have a TetrahedralGridDataset that we could slice twice?
There was a problem hiding this comment.
The logic of the bandgap monitor is that we gather all fields (Ec, Ev, Efn, etc.) together and do operations like plane_slice() and plot_slice() on all variables. So, plot_slice() works only for a 2D data and plots the 1D cross-section for all fields, and plane_slice() works only for a 3D data and make a 2D crossection -> new bandgap monitor data object that we can use for plot_slice() etc.
There was a problem hiding this comment.
If user wants to do more detailed analysis, the original data fields (and all related operations) are available.
|
Regarding the unit tests. I want to add one, but I don't understand the logic of the existing unit tests for SteadyPotentialMonitor, SteadyFreeCarrierMonitor, etc. It looks like they just test creation of the new data. |
| if "voltage" not in sel_kwargs: | ||
| raise DataError("'voltage' is not selected for the plot.") |
There was a problem hiding this comment.
We need to be a bit careful with this. If you run for a single voltage you should be able to get away without selecting a voltage
|
@vasilyzabelin with regards to the test, in those tests you're referring to I think we simply make sure we can create the structures. You can also test that the correct error is raised, For instance, you can check that if your data is of type |
|
I've added the unit test for the new monitor type. |
| if ("voltage" not in sel_kwargs) and (self.Ec.values.coords.sizes["voltage"] > 1): | ||
| raise DataError( | ||
| "'voltage' is not selected for the plot with multiple voltage data points." | ||
| ) |
There was a problem hiding this comment.
Can you check that this error is actually raised in your tests?
There was a problem hiding this comment.
I tried to do it, but I can't figure out the proper way. Can we have a meeting to discuss the logic of the unit tests here?
There was a problem hiding this comment.
Here's an example of what I mean:
tidy3d/tests/test_components/test_heat_charge.py
Line 1320 in 3dc27f1
though there are more examples in that same file. Basically, we set up the component in the way we want the error to arise and check that it does. We can of course discuss it in a meeting. Feel free to schedule one
There was a problem hiding this comment.
I will try. As far as I see, the monitor data in various tests here is artificially generated, and not a result of an actual simulation, that is done in this function:
So, I need to create the monitor data with the desired error?
There was a problem hiding this comment.
yes, you can create the data. There's examples in here:
tidy3d/tests/test_data/test_datasets.py
Line 672 in 3dc27f1
| if ("voltage" not in sel_kwargs) and (self.Ec.values.coords.sizes["voltage"] > 1): | ||
| raise DataError( | ||
| "'voltage' is not selected for the plot with multiple voltage data points." | ||
| ) |
There was a problem hiding this comment.
yes, you can create the data. There's examples in here:
tidy3d/tests/test_data/test_datasets.py
Line 672 in 3dc27f1
| if isinstance(self.Ec, TetrahedralGridDataset): | ||
| if N_coords != 2: | ||
| raise DataError( | ||
| "2 spatial coordinate values have to be defined to plot the 1D cross-section figure for a 3D dataset." | ||
| ) | ||
|
|
||
| elif isinstance(self.Ec, TriangularGridDataset): | ||
| if N_coords != 1: | ||
| raise DataError( | ||
| "1 spatial coordinate value has to be defined to plot the 1D cross-section figure for a 2D dataset." | ||
| ) | ||
|
|
||
| for index, coord_name in enumerate(["x", "y", "z"]): | ||
| if coord_name in selection_data: | ||
| axis = index | ||
| continue | ||
|
|
||
| if axis == self.Ec.normal_axis: | ||
| raise DataError( | ||
| f"Triangular grid (normal: {self.Ec.normal_axis}) cannot be sliced by a parallel plane." | ||
| ) |
There was a problem hiding this comment.
Let's add a test for these errors too
momchil-flex
force-pushed
the
vasily/bandgap_monitor_update
branch
from
698ed84 to
e695d6e
Compare
Update of the energy bandgap monitor for the CHARGE solver