Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scene/AbstractSimualtion/Simulation/HeatSimulation cleaning #1211

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

dbochkov-flexcompute
Copy link
Contributor

This PR switches optical classes (Simulation, Monitor, Source, MonitorData, SimulationData) to use introduced with the heat solver Scene, AbstractSimulation, AbstractMonitor, AbstractSource, AbstractMonitorData, AbstractSimulationData. The main changes are related to plotting functionalities. That is most of actual plotting functions and their helpers are now only in Scene. The overall distribution of plotting functions is the following

Scene:

  • plot_structures
  • plot_structures_eps
  • plot_structures_heat_conductivity
  • plot
  • plot_eps
  • plot_heat_conductivity

AbstractSimulation:

  • plot_sources
  • plot_monitors
  • plot_symmetries
  • plot_boundaries (just abstractmethod, actual implementation is deferred to solvers)
  • plot (works if plot_boundaries is defined in solver class)

Simulation:

  • plot_structures_eps (wrapper, for backward compatibility + to provide fdtd grid)
  • plot_structures (wrapper, for backward compatibility)
  • plot_eps (similar to Scene.plot_eps but include monitors, sources, etc)
  • plot_pml
  • plot_boundaries
  • plot_grid
  • plot (have to override AbstractSimulation.plot to add plot_pml)

HeatSimulation:

  • plot_heat_conductivity (similar to Scene.plot_heat_conductivity but include monitors, sources, etc)
  • plot_boundaries
  • plot_sources (have to override AbstractSimulation.plot_sources because sources work quite different in heat)

As I mentioned previously, in Scene.plot_structures_eps() I had to modify how custom materials are plotted because there is no fdtd grid. I added an optional argument grid: Grid so that in Simulation.plot_eps fdtd grid Simulation.grid is passed and custom materials are plotted as before.

Regarding wrappers Simulation.plot_structures_eps() and Simulation.plot_structures(): I added them to preserve backward compatibility and the difference between them and their Scene counterparts is in the default plot limits: in scene those are detected automatically based on structures, while in simulation the size of simulation domain is used. Plus in Simulation.plot_structures_eps() we also pass fdtd grid as described above. I think we should do either of two things:

  • eventually depreciate them and print a warning to use Scene.plot_structures_eps() and Scene.plot_structures()
  • or make it uniform across all solvers, that is, define them plus .plot_structures_heat_conductivity() in AbstractSimulation

Some other small things:

  • Simulation has .bounds_pml to represent actual simulation domain, but to have something uniformity between solvers I added property AbstractSimulation.simulation_bounds which by default are computed from .center and .size. And in Simulation its overridden using .bounds_pml. Do we want to depreciate .bounds_pml eventually?
  • as many validators as possible have been moved from Simulation to Scene and AbstractSimulation.
  • some properties and funcitons like mediums, medium_map, background_structure, intersecting_mediums, intersecting_structures are in Scene now, but to preserve backward compatibility there are repeated using dummy wrappers in Simulation. Similar questions, do we want to depreciate them in Simulation or extend to other solvers?
  • I removed some of code that was introduced with heat solver but turns out not currently needed, so some changes could be a bit confusing

@tylerflex
Copy link
Collaborator

tylerflex commented Oct 20, 2023

Regarding Simulation.plot_structures... wrappers. I think it might make sense to think about how to structure this generally in the AbstractSimulation. Just a quick thought: plot_structures should just plot the geometry of structures with each color signifying a unique Medium. Then I think (in 3.0 maybe) we can have some Scene methods plot_structures_x that plot the geometry and some color depending on the medium property x we are interested in. eg. plot_structures_eps displays the permittivity of each Geometry, plot_structures_heat_conductivity displays the heat_conductivity. We could also pass these as kwargs to plot_structures and unify everything. plot_structures(..., medium_display="permittivity"). For now I think how you have it set up is fine though. maybe once we get closer to doing 3.0 we can start thinking about how to unify these in AbstractSimulation, but it might be too early to worry about it now

Do we want to depreciate .bounds_pml eventually?

I think so, but probably not until 3.0. Maybe in the meantime, we can have it call .simulation_bounds for backwards compatibility.

[regarding Simulation properties] Similar questions, do we want to depreciate them in Simulation or extend to other solvers?

Kind of like the other two responses, I think it would be best to try to unify these concepts for all types of solvers for 3.0 but just support them like you have here for now (just for backwards compatibility). When we release 3.0 we can rename or remove these properties but yea I think it would be nice to unify them eventually.

These are my thoughts before digging into the code, so probably will have more later.

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbochkov-flexcompute It's of course hard to follow all of the individual changes, but this looks really good as I look through the diff and if the tests pass and it works in the notebooks, I think it's probably done correctly.

My only questions are about the file structure and whether we should try to reorganize the files now while we're doing this restructuring.

So now it seems that tidy3d/components contains

base_sim/
heat/
geometry/
grid/

and then pretty much everything else is a file. I'm wondering if we should try re-organizing this like we discussed into some major directories, such as

# anything generic that the components depend on.
base/
    constants.py
    types.py
    base.py
    data/
        data_array.py
        dataset.py
scene/
    scene.py
    geometry/
    structures.py
    medium.py
simulation/
    grid/
    base_sim/
    optical/
    heat/
    viz.py (?)
sim_data/ (or this data could be combined in the individual sims above)
    sim_data.py
    monitor_data.py

we could also consider renaming components to base or src. Just wondering if you think there's any restructuring structure in the filesystem that makes sense to just take care of now that we are doing this major refactor.

@tylerflex
Copy link
Collaborator

just want to say, if we want to do this more general refactoring after this PR (to split up the work) I'm fine with that too.

@dbochkov-flexcompute
Copy link
Contributor Author

I think it would be better to do file restructuring in a separate PR, because chances there could be other PRs that would need to be rebased after this PR is merged. It could be more confusing if it involves both modifications to classes and major file reshuffling

@dbochkov-flexcompute
Copy link
Contributor Author

  • rebased to the latest pre/2.5
  • moved plot_structures, plot_structures_eps (+ added plot_structures_heat_conductivity) from Simulation to AbstractSimulation. So both Scene and all simulation classes have functions plot_structures, plot_structures_eps, and plot_structures_heat_conductivity. The difference is that in Scene default plotting limits are determined automatically, while in simulation classes .center and .size is used.
  • add depreciation warnings to Simulation fields mediums, medium_map, background_structure, intersecting_mediums, intersecting_structures, eps_bounds
  • Corresponding backend PR to switch from depreciated properties/functions https://github.com/flexcompute/tidy3d-core/pull/443
  • Tested a few notebooks to make sure not errors/plotting changes
  • backend tests are passing too

@momchil-flex momchil-flex added the rc3 3rd pre-release label Oct 30, 2023
@momchil-flex momchil-flex merged commit 11b59e7 into pre/2.5 Oct 30, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rc3 3rd pre-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants