Add subsection for EMESimulation

[caseyflex]

This PR:

1. Moves subsection from Simulation to AbstractYeeGridSimulation.
2. Extends subsection in EMESimulation to also allow inheriting the EME grid spec. The behavior when eme_grid_spec == "identical" is to extend the region to snap to EME cell boundaries.
3. Adds validator for EME simulation to be fully 3D
4. Makes ModeSolver.reduced_simulation_copy use ModeSolver.to_fdtd_mode_solver() when the simulation is an EMESimulation. This is needed for now because an EMESimulation must be fully 3D, while we need a 2D subsection in the mode solver. Eventually, instead of using subsection of FDTDSimulation, it might be nicer to convert to a ModeSimulation and use subsection of ModeSimulation. But that would be after the ModeSimulation PR is merged.
5. Makes data_on_yee_grid always use the reduced_simulation_copy.

[caseyflex]

@weiliangjin2021 @momchil-flex I am tempted to make _data_on_yee_grid in the mode solver always use reduced_simulation_copy. Do you think there is any problem with this?

[momchil-flex]

@weiliangjin2021 @momchil-flex I am tempted to make _data_on_yee_grid in the mode solver always use reduced_simulation_copy. Do you think there is any problem with this?

If frontend and backend tests all pass I think it should be good?

[caseyflex]

@weiliangjin2021 @momchil-flex I am tempted to make _data_on_yee_grid in the mode solver always use reduced_simulation_copy. Do you think there is any problem with this?

If frontend and backend tests all pass I think it should be good?

Cool, I made the change. I'll run the backend tests some time today to confirm

[weiliangjin2021]

I am tempted to make _data_on_yee_grid in the mode solver always use reduced_simulation_copy. Do you think there is any problem with this?

Curious about the reason behind this?

I have a high-level code architecture question regarding this change: in some places we update ModeSolver object directly by swapping its simulation field to be the reduced simulation; now here with this change, some class methods themselves will reduce the simulation. So overall, the simulation can be reduced twice. Another major concern is that class methods are employing different simulations, which can be confusing in future development.

I wonder if it's possible to either 1) before running any mode solver, update its simulation field, or 2) all class methods in mode solver take reduced simulation.

[caseyflex]

I am tempted to make _data_on_yee_grid in the mode solver always use reduced_simulation_copy. Do you think there is any problem with this?

Curious about the reason behind this?

I have a high-level code architecture question regarding this change: in some places we update ModeSolver object directly by swapping its simulation field to be the reduced simulation; now here with this change, some class methods themselves will reduce the simulation. So overall, the simulation can be reduced twice. Another major concern is that class methods are employing different simulations, which can be confusing in future development.

I wonder if it's possible to either 1) before running any mode solver, update its simulation field, or 2) all class methods in mode solver take reduced simulation.

I don't want to update the simulation field -- I think it is reasonable for the user to expect that the simulation field of the mode solver will remain the simulation that they put in. I just want to internally use reduced_simulation_copy to improve performance, same as your backend PR. I chose to put that in _data_on_yee_grid, but it could just as well have gone in _solve_single_freq or even _solver_eps or _get_epsilon. Maybe you have an opinion about which place is best?

You also mention worries about reducing the simulation twice. I thought that the reduction operation should be idempotent -- doing it twice is the same as doing it once. Is that incorrect? If so, that might raise a concern about the semantics of reduction.

Overall, I don't want to change the simulation field of the ModeSolver, I just want to use the reduced_simulation_copy wherever data is being computed, since the data factors through the reduction. Ultimately, I would like to have a ModeSimulation class that could be a 2D simulation, or it could be a 3D simulation; in the latter case, it is preferable to keep the full simulation that the user input for plotting or other purposes, even if we internally use the reduced version for calculation.

[weiliangjin2021]

I don't want to update the simulation field -- I think it is reasonable for the user to expect that the simulation field of the mode solver will remain the simulation that they put in. I just want to internally use reduced_simulation_copy to improve performance, same as your backend PR. I chose to put that in _data_on_yee_grid, but it could just as well have gone in _solve_single_freq or even _solver_eps or _get_epsilon. Maybe you have an opinion about which place is best?

Good point. The backend change happens to be all internal. I guess in the future we can revert the backend change, and just update those methods to use reduced simulation?

You also mention worries about reducing the simulation twice. I thought that the reduction operation should be idempotent -- doing it twice is the same as doing it once. Is that incorrect? If so, that might raise a concern about the semantics of reduction.

Correct. Just feel unnecessary of doing it twice.

[caseyflex]

I don't want to update the simulation field -- I think it is reasonable for the user to expect that the simulation field of the mode solver will remain the simulation that they put in. I just want to internally use reduced_simulation_copy to improve performance, same as your backend PR. I chose to put that in _data_on_yee_grid, but it could just as well have gone in _solve_single_freq or even _solver_eps or _get_epsilon. Maybe you have an opinion about which place is best?

Good point. The backend change happens to be all internal. I guess in the future we can revert the backend change, and just update those methods to use reduced simulation?

You also mention worries about reducing the simulation twice. I thought that the reduction operation should be idempotent -- doing it twice is the same as doing it once. Is that incorrect? If so, that might raise a concern about the semantics of reduction.

Correct. Just feel unnecessary of doing it twice.

Ah, good point -- with this change in the frontend, the backend PR is no longer needed?

I think I'll leave it in _data_on_yee_grid which seems like it should always be called once when running the mode solver.

[weiliangjin2021]

Ah, good point -- with this change in the frontend, the backend PR is no longer needed?

Yeah, the backend PR is indeed not needed now, as you are using the reduced simulation in computing the modes. Verified in the slow custom medium example.

[caseyflex]

@momchil-flex can we merge this? I made some comments on the backend PR about memory estimation, but ultimately I don't think any backend updates are needed here, and the current memory estimation should remain valid?

[momchil-flex]

Yeah, looks good. Just remove the modification to the notebooks submodule.

(Note I recently had a message about this on slack too, you can refer to it if unsure about the reasoning / workflow going forward.)

[momchil-flex]

Let's also add a changelog item to changed saying that the mode solver now always works on a reduced simulation copy

[caseyflex]

ok, both fixed

[tylerflex]

looks good, but I'm confused about a validator _validate_size

[tylerflex]

hm, shouldn't this return val if it works? I'm a bit confused

[caseyflex]

good catch. The validator was overwritten by another validator with the same name below, so it was not being run. I added a few more tests to catch this

[caseyflex]

often, other errors were being raised, say when the mode solver monitors were not 2D. but now I test zero size in transverse direction too

[tylerflex]

ah ok 👍 approving