Vasily/bandgap_monitor_update #2344

vasilyzabelin · 2025-04-01T02:02:55Z

Update of the energy bandgap monitor for the CHARGE solver

momchil-flex

Seems like it still needs a test to be added as per my comment on the other PR.

marc-flex

Thanks Vasily for this! My main comment would be the platting function and how this would work for a 3D monitor. But other than that looks good to me

marc-flex · 2025-04-02T08:43:55Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        field_data = {field: values.get(field) for field in ["Ec", "Ev", "Ei", "Efn", "Efp"]}
+
+        for field, data in field_data.items():


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

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.

marc-flex · 2025-04-02T08:51:52Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        if not isinstance(self.Ec, TriangularGridDataset):
+            raise DataError(
+                "Bandgap monitor slice plot can be done only for a 2D unstructured dataset."
+            )


Do we really need this restriction? If we had a 3D monitor we would have a TetrahedralGridDataset that we could slice twice?

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.

If user wants to do more detailed analysis, the original data fields (and all related operations) are available.

marc-flex · 2025-04-02T09:00:51Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        if not isinstance(self.Ec, TetrahedralGridDataset):
+            raise DataError(
+                "Bandgap monitor plane slice can be done only for a 3D unstructured dataset."
+            )
+


So if I understand correctly, the user would first need to create a copy of the data using this plane_slice(axis, pos) and then plot_slice(axis, pos). Wouldn't it make sense to have a plot function that would take all? So for instance, in the case of a 3D monitor it could be plot_diagram(x=1,y=3) and when a 2D plot_diagram(x=1)?

It can be done this way.
But in the current implementation we need only two specific operations for the bandgap monitor with the syntax that is very close to operations for .sel() function and plane_slice(). So it is already familiar for users.

I think it'd be nicer to have it all in one function so that the user doesn't need to go through a series of steps instead of doing it in a single step.

So basically:

If data is 3D

user specifies only one coordinate (e.g., x=2) -> raise error

user specifies 2 coordinates (e.g., x=2, z=4) -> OK

if data is 2D

user specifies only one coordinate (e.g., x=2) -> OK

user specifies 2 coordinates (e.g., x=2, z=4) -> raise error

Yeah that would also make sense and is along the lines of our field plotting, again bringing up the sel_kwargs there:

tidy3d/tidy3d/components/data/sim_data.py

Lines 488 to 493 in 382f204

sel_kwargs : keyword arguments used to perform ``.sel()`` selection in the monitor data.

These kwargs can select over the spatial dimensions (``x``, ``y``, ``z``),

frequency or time dimensions (``f``, ``t``) or ``mode_index``, if applicable.

For the plotting to work appropriately, the resulting data after selection must contain

only two coordinates with len > 1.

Furthermore, these should be spatial coordinates (``x``, ``y``, or ``z``).

In that case, we expect exactly two spatial coordinates to remain after all the selection is done. Here, we expect one.

OK. I will update the code.

vasilyzabelin · 2025-04-03T00:07:50Z

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.

marc-flex

Thanks Vasily!
For the naming of the function maybe plot would suffice? Other than that I think this looks pretty much ready

tidy3d/components/tcad/data/monitor_data/charge.py

marc-flex · 2025-04-07T08:02:25Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        if "voltage" not in sel_kwargs:
+            raise DataError("'voltage' is not selected for the plot.")


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

tidy3d/components/tcad/data/monitor_data/charge.py

marc-flex · 2025-04-07T08:34:53Z

@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 TetrahedralGridDataset and only one selection coordinate is given, a DataError is raised.

vasilyzabelin · 2025-04-08T02:23:37Z

I've added the unit test for the new monitor type.

marc-flex

Overall this looks good but it'd be great if you could update the test to capture the error when voltage isn't provide but we have fields with multiple voltages

marc-flex · 2025-04-09T08:59:14Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        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."
+            )


Can you check that this error is actually raised in your tests?

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?

Here's an example of what I mean:

tidy3d/tests/test_components/test_heat_charge.py

Line 1320 in 3dc27f1

with pytest.raises(KeyError):

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

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:

tidy3d/tests/test_components/test_heat_charge.py

Line 199 in 3dc27f1

def monitors():

So, I need to create the monitor data with the desired error?

yes, you can create the data. There's examples in here:

tidy3d/tests/test_data/test_datasets.py

Line 672 in 3dc27f1

def test_cell_values():

marc-flex

This is close to finished. I guess at this point is only a matter of adding corresponding tests

marc-flex · 2025-04-11T14:29:42Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        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."
+            )


yes, you can create the data. There's examples in here:

tidy3d/tests/test_data/test_datasets.py

Line 672 in 3dc27f1

def test_cell_values():

marc-flex · 2025-04-11T14:31:09Z

tidy3d/components/tcad/data/monitor_data/charge.py

+        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."
+                )


Let's add a test for these errors too

vasilyzabelin added 5 commits March 24, 2025 21:35

Add energy bandgap monitors for the CHARGE simulations.

d7c3523

Update the energy band monitor code

c64873a

Add energy bandgap monitors for the CHARGE simulations.

34135ce

Merge branch 'temp_branch' into HEAD

55acc38

Update of the bandgap monitor for the CHARGE solver

eead787

vasilyzabelin requested review from marc-flex and momchil-flex April 1, 2025 02:04

vasilyzabelin mentioned this pull request Apr 1, 2025

Add energy bandgap monitors for the CHARGE simulations. #2337

Open

momchil-flex closed this Apr 1, 2025

momchil-flex reopened this Apr 1, 2025

momchil-flex reviewed Apr 2, 2025

View reviewed changes

marc-flex reviewed Apr 2, 2025

View reviewed changes

Update the plot function for the bandgap monitor.

630eecf

marc-flex reviewed Apr 7, 2025

View reviewed changes

Minor update of the badgap monitor.

fdb6350

marc-flex approved these changes Apr 9, 2025

View reviewed changes

marc-flex reviewed Apr 9, 2025

View reviewed changes

vasilyzabelin added 2 commits April 9, 2025 09:57

Minor code update

9006c53

Minor code update

a7998fb

marc-flex requested changes Apr 11, 2025

View reviewed changes

marc-flex added the Charge label Apr 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vasily/bandgap_monitor_update #2344

Vasily/bandgap_monitor_update #2344

vasilyzabelin commented Apr 1, 2025

momchil-flex left a comment

marc-flex left a comment

marc-flex Apr 2, 2025

vasilyzabelin Apr 2, 2025

marc-flex Apr 2, 2025

vasilyzabelin Apr 2, 2025

vasilyzabelin Apr 3, 2025

marc-flex Apr 2, 2025

vasilyzabelin Apr 3, 2025

marc-flex Apr 3, 2025

marc-flex Apr 3, 2025

momchil-flex Apr 3, 2025

vasilyzabelin Apr 3, 2025

vasilyzabelin commented Apr 3, 2025

marc-flex left a comment

marc-flex Apr 7, 2025

marc-flex commented Apr 7, 2025

vasilyzabelin commented Apr 8, 2025

marc-flex left a comment •

edited

Loading

marc-flex Apr 9, 2025

vasilyzabelin Apr 9, 2025

marc-flex Apr 9, 2025

vasilyzabelin Apr 9, 2025

marc-flex Apr 11, 2025

marc-flex left a comment

marc-flex Apr 11, 2025

marc-flex Apr 11, 2025

		field_data = {field: values.get(field) for field in ["Ec", "Ev", "Ei", "Efn", "Efp"]}

		for field, data in field_data.items():

	sel_kwargs : keyword arguments used to perform ``.sel()`` selection in the monitor data.
	These kwargs can select over the spatial dimensions (``x``, ``y``, ``z``),
	frequency or time dimensions (``f``, ``t``) or ``mode_index``, if applicable.
	For the plotting to work appropriately, the resulting data after selection must contain
	only two coordinates with len > 1.
	Furthermore, these should be spatial coordinates (``x``, ``y``, or ``z``).

		if "voltage" not in sel_kwargs:
		raise DataError("'voltage' is not selected for the plot.")

Vasily/bandgap_monitor_update #2344

Are you sure you want to change the base?

Vasily/bandgap_monitor_update #2344

Conversation

vasilyzabelin commented Apr 1, 2025

momchil-flex left a comment

Choose a reason for hiding this comment

marc-flex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasilyzabelin commented Apr 3, 2025

marc-flex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-flex commented Apr 7, 2025

vasilyzabelin commented Apr 8, 2025

marc-flex left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-flex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-flex left a comment •

edited

Loading