tidy3d icon indicating copy to clipboard operation
tidy3d copied to clipboard

Add ModeSimulation classes

Open caseyflex opened this issue 1 year ago • 5 comments

This PR introduces ModeSimulation classes as a new interface for the mode solver. The ModeSimulation classes more closely parallel Simulation and EMESimulation (and inherit a lot of the functionality from AbstractYeeGridSimulation). This makes a lot of user-facing operations more convenient.

The ModeSimulation can be 2D or 3D; if it is 3D, the mode plane must be specified as well. There are functions to convert to and from other AbstractYeeGridSimulation types. The existing ModeSolver plugin still works, but is just a wrapper around the ModeSimulation classes.

Todo:

  • [x] Move solve and data operations to ModeSimulation or ModeSimulationData classes.
  • [ ] improve test coverage a bit
  • [ ] Check consistency in plotting functions
  • [ ] Check that memory usage and performance is mostly unchanged

caseyflex avatar Jul 17 '24 07:07 caseyflex

@tylerflex (and others): Currently, ModeSimulation does not have sources or monitors. It just treats the entire ModeSimulation as the plane for mode solving, and saves the full ModeSolverData in ModeSimulationData.modes.

I will add interval_space and fields parameters to the ModeSimulation class to allow for downsampling here.

caseyflex avatar Jul 17 '24 07:07 caseyflex

Thanks @caseyflex , sorry it took so long to get to this. Looks overall good.

One question is whether we want the ModeSimulation to support local solves (using the mode solver internals moved into components). and then the plugin would be a light wrapper on this?

How it's set up now also works, but just want to throw that out there.

Yeah, I think that's a good question, whether we want ModeSimulation to support local solves. Currently, you would have to create a ModeSolver and set simulation to the ModeSimulation -- you no longer need an FDTD simulation to do mode solves, but you would still have to use the ModeSolver plugin.

We certainly could move all the mode solver internals to the ModeSimulation directory / class, and make the plugin just a thin wrapper around that. I have one major concern, relating to the autogrid. If you make a ModeSimulation with autogrid, it will generate its own grid which can be used for mode solving just fine. However, another use case is an FDTD simulation, and you want to mode solve in just a planar region of the simulation. In this case, you want to use the grid coming from the entire FDTD simulation, and not just allow the mode solver region to generate its own grid. In spite of that, we could move the core mode solver code from plugins to components, inside the mode directory, and make it closely integrated with ModeSimulation, which could then have an easier local solve api; however, we would still want the mode solver internals to operate on a simulation which could be a Simulation, EMESimulation, or ModeSimulation, and not restrict it to operating on a ModeSimulation. Although it is possible to restructure in this way, this might be an argument against. Another argument is that it's unfortunate to keep moving stuff to components (should the mode solver be a separate repo?). A final argument is that we greatly prefer the remote mode solver for accuracy reasons; but the local one is definitely a huge convenience.

caseyflex avatar Jul 25 '24 09:07 caseyflex

Yeah, I think that's a good question, whether we want ModeSimulation to support local solves. Currently, you would have to create a ModeSolver and set simulation to the ModeSimulation -- you no longer need an FDTD simulation to do mode solves, but you would still have to use the ModeSolver plugin. We certainly could move all the mode solver internals to the ModeSimulation directory / class, and make the plugin just a thin wrapper around that.

All else equal, being able to do local without needing the plugin sounds best to me.

I have one major concern, relating to the autogrid. If you make a ModeSimulation with autogrid, it will generate its own grid which can be used for mode solving just fine. However, another use case is an FDTD simulation, and you want to mode solve in just a planar region of the simulation. In this case, you want to use the grid coming from the entire FDTD simulation, and not just allow the mode solver region to generate its own grid. In spite of that, we could move the core mode solver code from plugins to components, inside the mode directory, and make it closely integrated with ModeSimulation, which could then have an easier local solve api; however, we would still want the mode solver internals to operate on a simulation which could be a Simulation, EMESimulation, or ModeSimulation, and not restrict it to operating on a ModeSimulation. Although it is possible to restructure in this way, this might be an argument against.

Hm, maybe I dont fully understand. So you're saying that in some sense maybe it makes sense for the mode solver to be not its own Simulation class, but rather some kind of "run" function that gets applied to other simulation types? For example, have run(sim: Simulation) -> SimulationData and run_mode_solver(sim: Simulation) -> ModeSolverData ?

And this would I guess cause a pretty big overhaul of the code architecture?

Can some of this be alleviated by importing grids from Simulation or EMESimulation ie with https://github.com/flexcompute/tidy3d/pull/1836, or by providing to_ and from_ methods to allow importing and exporting ModeSimulation from other simulation types (while preserving grids?)

Another argument is that it's unfortunate to keep moving stuff to components (should the mode solver be a separate repo?).

I think as long as the mathematical bits are sufficiently abstracted away from the pydantic / api, it should be ok. but yea we've sort of been moving in this integrated components approach for some time now (eg with HeatSimulation) so I feel it's maybe a more general concern? I think we should reduce coupling as much as we can, but at the same time, sometimes having separate plugins creates its own issues all the same.

A final argument is that we greatly prefer the remote mode solver for accuracy reasons; but the local one is definitely a huge convenience.

I use the local one quite a bit, since a lot of times my remote mode solves (even when small) get queued and it can be a bit annoying to wait for them, especially when I've already run them previously.

tylerflex avatar Jul 25 '24 13:07 tylerflex

I haven't digested everything here but real quick comments:

However, another use case is an FDTD simulation, and you want to mode solve in just a planar region of the simulation. In this case, you want to use the grid coming from the entire FDTD simulation, and not just allow the mode solver region to generate its own grid.

The current ModeSolver has a .reduced_simulation_copy method which makes a copy of the mode solver that only has a span as big as its plane, and throws away structures that are not interesected by that plane, and even throws away custom medium data that is outside of what's needed for the interpolation needed in the solver. There's some tricky details about getting the discretization right to match the simulation grid exactly - in fact @weiliangjin2021 has a PR that's ready to be merged that improves exactly on that aspect. The original motivation for this was exactly the case where a large amount of custom medium data needs to be uploaded for a mode solver that only requires a small section of it.

The use case of defining a mode solver within an FDTD simulation (on the same grid as the FDTD simulation) is definitely a very common one, including internally for the functioning of our mode sources and monitors. I think the obvious way to handle this in the ModeSimulation is to have a similar method to the .reduced_simulation_copy (probably reusing a lot of the code), but since ModeSimulation no longer stores an FDTD simulation inside of it, it would rather be a classmethod, something like ModeSimulation.from_simulation(simulation, **mode_kwargs). This would then produce a ModeSimulation instance with the grid exactly matching the one of the input simulation. I mean, I'm saying the obvious way, but there will probably be some pitfalls to be wary of.

Regarding local run, yeah, it's definitely important to still support... Maybe we should limit the scope for now though and first work on the API and the web run, while still allowing a local run for debug purposes through a conversion to ModeSolver? Then separately work on bringing the plugin solver code into the core tidy3d components. As you refer.

momchil-flex avatar Jul 25 '24 16:07 momchil-flex

Thanks for the feedback @tylerflex @momchil-flex . I'm still thinking about whether I should move the core mode solver code into components for this PR or wait until later. Like you said, this would be helpful but it might be better to limit the scope of this PR. However, if it would make the code a lot nicer / avoid code duplication, it may be preferable to do it here.

I looked into the PRs on subsection and reduced_simulation_copy. I think it makes sense to have something like that, ModeSimulation.from_simulation. I see that @weiliangjin2021 's PR matching the grid exactly is very helpful for this. Thinking about these details, I realized another technical concern. I currently made the ModeSimulation a 2D simulation, because that's intuitively how the mode solver works. But I see that reduced_simulation_copy actually preserves an extra cell on either side of the mode solver plane, so that subpixel can work. The comments say it is for subpixel for custom medium, but I'm not sure I understand exactly what is going on there. I suppose for ModeSimulation we could either have a 2D simulation, or a 3D simulation with a specified mode solving plane? This would allow the most flexibility for both use cases.

caseyflex avatar Jul 29 '24 12:07 caseyflex