Added EME solver

[caseyflex]

I left some comments below about some key aspects of the organization.

Todo:

• Fix derived fields plotting
• Disallow fully anisotropic materials. Warn about other advanced materials.
• Unit tests
• Subgrid visualization
• plot_grid
• precision in storage_size
• plot port modes and mode solver modes and coeffs
• mode_indices -> num_modes in monitors
• finalize or get rid of methods for converting to / from fdtd sim
• Create AbstractYeeGridSimulation as abstract superclass of EMESimulation and Simulation, and similarly for sim_data.

[caseyflex]

This class is different than ScalarModeFieldDataArray in that it has an additional port_index dimension. Should validate that it is always has coords [0, 1].

There is an issue that num_modes may be different at the two ports. Current plan is to have nans in the dataarray when the mode_index, port_index pair doesn't make sense. The smatrix plugin has this same issue, I assume it handles it similarly.

[momchil-flex]

The S matrix doesn't really have a field monitor, so not sure if there's anything there to which the comment applies?

NaN may be fine. An alternative path could be to remove port_index from this (which would actually make it equivalent to ScalarModeFieldDataArray in terms of dimensions, but it could be kept separate because the associated physical meaning is different). Then EMEFieldData could become EMEPortFieldData and EMEFieldData (i.e. what is eventually returned by an EMEFieldMonitor) could carry two instances of EMEPortFieldData, something like eme_field_data.port0 and eme_field_data.port1. Not sure if this (or something along these lines) makes sense though....

[caseyflex]

For the smatrix plugin, I'm referring to this:

tidy3d/tidy3d/plugins/smatrix/component_modelers/modal.py

Line 302 in e5c1439

max_mode_index_out, max_mode_index_in = self.max_mode_index

Each port may have a different number of modes, so it just uses the max when setting up the DataArray coords.

[caseyflex]

yeah, I actually initially had it behave something like that. The difficulty I had was that I wanted EMEFieldData to inherit from ElectromagneticFieldData. In this setup, EMEPortFieldData could inherit from ElectromagneticFieldData, except that there is supposed to be a one-to-one correspondence between Monitors and MonitorDatas.

[lucas-flexcompute]

My comment here might be related to this discussion.

[caseyflex]

port_index in EMEFieldData labels the actual device port. EMEFieldData has nothing to do with the EME grid, it is rather defined on the FDTD grid.

[caseyflex]

Unfortunately, I had to copy this from components/sim_data.py. Would it make sense to move some of this field plotting functionality into base_sim?

[momchil-flex]

I think what would rather make sense is to have some intermediate level abstract class from which both EME and FDTD simulations inherit (and probably mode in the future, as ModeSolver has a plot_field method too), but not Heat and e.g. charge in the future. It is either that or decoupling the plot function completely in 3.0, something like plot_field(sim_data, ...) instead of sim_data.plot_field(...). But for the time being an AbstractElectromagneticSimulationData with a plot_field method that both Simulation and EMESimultion inherit maybe makes most sense?

[momchil-flex]

@tylerflex @daquintero

[caseyflex]

that sounds reasonable to me

[tylerflex]

Interestingly, this is the kind of problem we're trying to fix for 3.0 :) Now that we have all these types of simulations, it gets really annoying to have to use inheritance to specify how all of these methods work.

I think the approach momchil suggested probably makes some sense for now.

However, how I'm thinking about it for 3.0 is that the simulation has some properties or methods that simply separate the optical parts out, and then we have some plot_optical or plot_thermal etc. that filter by these properties and plot them in a generic way.

Related, but I'm thinking that the Simulation and SimulationData might want to be unified in a single object, so SimulationData.plot_field(mnt_name) would just work as is now and just continue to error if the mnt_name was not a FieldMonitor.

[tylerflex]

sorry let me clarify, I dont mean that in 3.0 Simualtion gets unified with SimulationData. I mean that maybe we want to throw EME, Heat, FDTD, etc all into a single Simulation object (and same with their data) basically not distinguishing between the solvers besides the various monitors and sources that are contained within.

[caseyflex]

@tylerflex I added AbstractYeeGridSimulation and AbstractYeeGridSimulationData as discussed -- it saved a lot of code duplication. I generally tried to move everything related to epsilon, grids, and EM fields into these classes, while keeping things only relevant for time-domain in Simulation and things only relevant for EME in EMESimulation.

[tylerflex]

That's great! So overall it refactored pretty nicely? eg. without a lot of rough edges? If so, that's a good sign for the future. Next I'll implement the mode solver as an AbsttractYeeGridSimulation, probably it will reuse a lot of the same internals.

[caseyflex]

That's great! So overall it refactored pretty nicely? eg. without a lot of rough edges? If so, that's a good sign for the future. Next I'll implement the mode solver as an AbsttractYeeGridSimulation, probably it will reuse a lot of the same internals.

Yeah, I think it refactored nicely. I might still change a bit before EME pre-release (e.g. consolidating size validators).

[momchil-flex]

7000 lines PR, 5 words changelog :D

(I think that's fine, just funny)

[caseyflex]

:D

[momchil-flex]

I think I see why but out of curiosity what is the problem if this goes to interp given that the values should always be integer and effectively no interpolation should happen?

[caseyflex]

da = xr.DataArray(
    data=[[0, np.nan, np.nan], [1, np.nan, np.nan]],
    dims=("x", "y"),
    coords={"x": [0, 1], "y": [0, 1, 2]},
)
print(da.interp(y=0))
print(da.sel(y=0))

<xarray.DataArray (x: 2)> Size: 16B
array([nan, nan])
Coordinates:
  * x        (x) int64 16B 0 1
    y        int64 8B 0
<xarray.DataArray (x: 2)> Size: 16B
array([0., 1.])
Coordinates:
  * x        (x) int64 16B 0 1
    y        int64 8B 0

Seems to be a weird issue with xarray multidimensional interpolation when nans are present?

[momchil-flex]

Oh ok so I didn't actually see. :) Makes sense.

[momchil-flex]

This seems like quite a mess in the diff here but in practice it should be pretty small changes I assume? Could you summarize what happened?

[daquinteroflex]

Hmm from what I can tell it's just organising AbstractYeeGridSimulation and Simulation in the same file. I'm wondering (although maybe a bit painful) if it makes sense to move this to the base_sim/abstract_yee_grid maybe thingking that at some point 3.0 it might also make sense to rename our current simulation class FDTDSimulation although we can also move this later too! Maybe @tylerflex has some thoughts too

[caseyflex]

I moved all of the fields and methods that weren't FDTD-specific but involve EM fields on Yee grid to AbstractYeeGridSimulation. I left duplicates of some methods and fields in Simulation when their docstrings had information specific to FDTD simulations.

[daquinteroflex]

(Not related to this review specifically)

You know I was thinking about this fields/methods code duplicates issue recently. I believe we won't be able to escape it fundamentally in an inheritance-based data scheme (which is good and bad for different reasons), but the alternative which is strongly-typed methods that operate on data container classes have their own set of problems too I guess more on the "user-friendly linking methods to data classes" aspect. This might just be fundamental to having multiple classes have "slightly different variations of the same method" be it in docstrings or implementation. Problems can become more prevalent if we have classmethods using otherclassmethods in a breaking manner that I think we generally try to avoid by writing pure class functions. It is very strongly coupled to the way we choose to represent our data really and have users interact with it, and any approach involves considerable work. There might be a conceptual point of having to draw the line of how exactly we want to interact with our data, and in a way maybe these quirks could be part of it. The real question is the effect this has on scalability and integration with other methods, and how much this affects our development understanding and maintenance of the code. These are not easy questions but I think @tylerflex was thinking about this too in his concept

[momchil-flex]

Hmm from what I can tell it's just organising AbstractYeeGridSimulation and Simulation in the same file. I'm wondering (although maybe a bit painful) if it makes sense to move this to the base_sim/abstract_yee_grid maybe thingking that at some point 3.0 it might also make sense to rename our current simulation class FDTDSimulation although we can also move this later too! Maybe @tylerflex has some thoughts too

I think this makes sense, but it can be made in a separate PR. We could also go more closely over whether some of the code duplication may be avoided. I'm also down to rename it to FDTDSimulation. We may still do something like both import FDTDSimulation and import FDTDSimulation as Simulation in tidy3d init if we want to keep some backwards compatibility.

[momchil-flex]

This seems unnecessarily limiting? But I might see your reasoning since the computation can become quite heavy for a large number of freqs.

[caseyflex]

yeah, when we use a lot of frequencies we can run into performance issues and memory issues in the mode solver I believe

[lucas-flexcompute]

I'm guessing the EME sweep helpers will be worked on in another PR, right? In that case, this is looking good to me.

[daquinteroflex]

Docs structures look good so far!

[daquinteroflex]

11/10 job! Congratulations, so much effort here, and it's really good!

I made a few tiny comments that maybe we can also address in the 3.0 refactor really, but it's great

[daquinteroflex]

Maybe naive question. This is interesting, just out of curiousity, is this how it gets constructed in the backend too in the sense that if a user makes a custom grid fucntion based on box cells it would also work?

[caseyflex]

It's not far off. In principle the EME cells are just a list of cells, yes, so in that sense it can be decoupled from the actual simulation geometry a bit. This could be useful for periodic repetition of cells or other reuse of mode solving in the future

[daquinteroflex]

Hmm from what I can tell it's just organising AbstractYeeGridSimulation and Simulation in the same file. I'm wondering (although maybe a bit painful) if it makes sense to move this to the base_sim/abstract_yee_grid maybe thingking that at some point 3.0 it might also make sense to rename our current simulation class FDTDSimulation although we can also move this later too! Maybe @tylerflex has some thoughts too

[daquinteroflex]

This is maybe something we should add later, but I believe we would benefit from having a SolverSimulationType definition, also in the context of 3.0, so that all these methods can just use that type both at the docstring and parameter typing check level @tylerflex