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

Heat solver interface #1083

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Heat solver interface #1083

merged 1 commit into from
Oct 6, 2023

Conversation

dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Aug 18, 2023

Since the old PR (#896) became very outdated, I've decided to open a new one.

Major changes/additions (directory structure is up for discussion, just whatever seemed to make sense for now):

components/heat_spec.py:

All medium classes can now have field heat_spec which can be either SolidSpec or FuildSpec for specifying thermal properties. To remind, there are three categories of properties:

  1. optic (permittivity, conductivity, ...) - defined using standard Medium, Lorentz, etc classes
  2. thermal (heat conductivity, capacity, ...) - added through field heat_spec.
  3. response of optic properties to temperature (perturbations) - added when using PerturbationMedium and PerturbationPoleResidue classes.

And this is how different options interact with heat solver:

  • Structures using medium instances with heat_spec=None or heat_spec=FluidSpec() will not be included in solution domain for the heat equation, and they will not be converted to custom mediums after applying temperature fields.
  • Structures using non-perturbation medium instances with heat_spec=SolidSpec(...) will be included in solution domain for heat equation, but they will not be converted to custom mediums after applying temperature fields.
  • Structures using perturbation medium instances (PerturbationMedium and PerturbationPoleResidue) with heat_spec=SolidSpec(...) will be included in solution domain for heat equation, and they will be converted to custom mediums after applying temperature fields. (the fullest material description)

components/scene.py:

Carved out geometric only information from Simulation into class Scene for easier transfer of geometric information between different solvers. Basically, it contains structures, medium, center, size + plotting functionality.

  • Point to discuss: plotting of custom media is dependent on simulation grid in Simulation. I think we should still have eps plotting in Scene class (along with thermal conductivity plotting, and perhaps other properties in future), so I adjusted the plotting of custom medium to use custom medium sample points + structure bounds instead of simulation grid. But now the question is whether Simulation in future should just directly borrow this plotting method or to have its own based on actual simulation grid?
  • As discussed, for now we will not make any changes to Simulation, only added property Simulation.scene and class method Simulation.from_scene(scene: Scene, **kwargs) to transferring Scene between optic/heat simulation.
  • Contrary, HeatSimulation does have field scene: Scene

components/bc_placement.py:

This file defines classes for placement of boundary conditions on complex interfaces (BCPlacementType=[StructureBoundary, StructureStructureInterface, MediumMediumInterface, etc]). I realized this should be separated from heat solver components because it can be reused for other future solvers.

components/heat/:

The remaining of heat components are placed in this directory to keep everything tidy (yes, you see what I did here lol) and they include:

  • boundary.py boundary conditions specifications: TemperatureBC, HeatFluxBC, ConvectionBC + HeatBoundarySpec
  • grid.py heat grid specification: currently, UniformHeatGrid and DistanceHeatGrid. I can that this can go either way: we can introduce more classes to give the user more control or, contrary, unify everything into one spec so that the user don't have to think about what to select at all.
  • sim_data.py defines HeatSimulationData which is currently simply HeatSimulation instance + temperature SpatialDataArray
  • simulation.py defines HeatSimulation
  • source.py heat source: currently only UniformHeatSource
  • viz.py some parameters for plotting heat information

Notebooks with demonstrations

There are two draft notebooks demonstrating:

additional major things off the top of my head to discuss:

  • Currently boundary conditions and heat source placement specifications are done through using structures and medium names. For example, placement = td.StructureStructureInterface(structures=["box", "sphere"]). This doesn't seem ideal. We could maybe just pass structures/mediums themselves. But that will increase size of HeatSimulation instances. Maybe an alternative is to just record hash of passed structures/mediums? But then it would be harder to point user which structure/medium is missing from a heat simulation.

@tylerflex
Copy link
Collaborator

tylerflex commented Aug 22, 2023

  • Point to discuss: plotting of custom media is dependent on simulation grid in Simulation. I think we should still have eps plotting in Scene class (along with thermal conductivity plotting, and perhaps other properties in future), so I adjusted the plotting of custom medium to use custom medium sample points + structure bounds instead of simulation grid. But now the question is whether Simulation in future should just directly borrow this plotting method or to have its own based on actual simulation grid?

I think the approach you took makes sense. But I think in the future, a Simulation should probably modify this with the grid supplied. Would it make sense to modifying the medium plotting to add a method plot_with_grid() that accepts a grid and determines the plotting behavior based on the presence of a grid? eg. by default it can call the generic plot() but if a custom medium is inside of a simulation, it can call the current plotting method?

  • As discussed, for now we will not make any changes to Simulation, only added property Simulation.scene and class method Simulation.from_scene(scene: Scene, **kwargs) to transferring Scene between optic/heat simulation.
  • Contrary, HeatSimulation does have field scene: Scene

sounds good, were we also going to add a Simulation.from_scene(scene: Scene, **kwargs)? might be good to play around with defining Simulation from scenes before moving everything over.

  • Currently boundary conditions and heat source placement specifications are done through using structures and medium names. For example, placement = td.StructureStructureInterface(structures=["box", "sphere"]). This doesn't seem ideal. We could maybe just pass structures/mediums themselves. But that will increase size of HeatSimulation instances. Maybe an alternative is to just record hash of passed structures/mediums? But then it would be harder to point user which structure/medium is missing from a heat simulation.

What is the main problem with using names? I feel like it might be the ideal.
We could also consider a StructureReference with a name and hash field? So pass the object itself to structures but then store a hash of the structure and its name (if provided)?

@tylerflex tylerflex added the 2.5 label Aug 23, 2023
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.

Looks really good @dbochkov-flexcompute. Mostly minor comments and clarifications here.

tests/test_components/test_scene.py Outdated Show resolved Hide resolved
tidy3d/components/bc_placement.py Outdated Show resolved Hide resolved
tidy3d/components/bc_placement.py Outdated Show resolved Hide resolved
"""Abstract placement for boundary conditions."""


class StructureBoundary(AbstractBCPlacement):
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does it mean for a boundary condition to be on a structure boundary? Does it completely encase the structure geometry? And then if it's, say, structure-structure, it only exists at the intersection between the two geometries?

For example, if I specify a HeatFluxBoundaryCondition of flux=0 between two structures, that means no heat can flow between them?

tidy3d/components/heat/grid.py Show resolved Hide resolved
tidy3d/components/heat_spec.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/scene.py Outdated Show resolved Hide resolved
tidy3d/components/scene.py Outdated Show resolved Hide resolved
tidy3d/web/heat.py Outdated Show resolved Hide resolved
@joamatab
Copy link
Contributor

When will this heat solver be available?
we are interested in trying it out

at the moment from gdsfactory we can run heat solving using femwell

@HelgeGehring

@momchil-flex
Copy link
Collaborator

It turns out it's quite some effort to really nail everything down. This will not make it in the official 2.4.0 which will be released next week, but we will let you know as soon as it is available in a 2.5 pre-release so that you can start testing. This will be very valuable for us, too!

@dbochkov-flexcompute
Copy link
Contributor Author

dbochkov-flexcompute commented Sep 9, 2023

Major revision taking into account all comment. Most significant additions are PolySlab support, Symmetry support, and proper monitor/simulation data classes. It seemed to accomplish that the most natural way would be to create abstract base classes for main components since their optic and heat specification will share many things, I put them in base_sim/ for now. So, aiming at the following structure eventually for simulation classes (and similarly for other components):

      Scene
        |
  (composition)   # we can consider doing here inheritance as well
        |
AbstractSimulation(ABC)------------------
        |                               |
  (inheritance)                   (inheritance)
        |                               |
Simulation(AbstractSimulation)    HeatSimulation(AbstractSimulation)

Demonstration notebooks have been updated too:

One concerns: we chose to just copy relevant code from Simulation to Scene for now, thinking that just removing duplicating code would be easier if everything works out. But I start noticing that Simulation functionalities start diverging from whatever I have put in Scene. This could happen for other components. So, perhaps, we shouldn't wait too long until we actually reimplement Simulation and other optic classes based on introduced in this PR Scene and higher level abstractions.

@momchil-flex
Copy link
Collaborator

Yes, that sounds important. Let's aim to have that done for 2.5 too. Maybe even do the refactor as soon as we start a 2.5 branch?

@tylerflex
Copy link
Collaborator

tylerflex commented Sep 11, 2023

What do you have in mind regarding scene? I'm in favor of unifying them in 2.5 if it seems pretty likely we want to go forward with this approach for 3.0, but I'm worried about whether we can introduce scene into Simulation without breaking backwards compatibility. Maybe we'd need to add scene as a field and then do some validator magic to handle center, size, medium, and scene properly? for example

  • user can either specify scene directly, or
  • center, size, medium, which gets converted and stored as scene
  • provide @propertys for center size medium to enable previous functionality

or did you have something else in mind?

@momchil-flex
Copy link
Collaborator

I see, not so straightforward yeah, but in terms of backwards compatibility I think we'll need to do something like point 2. for now, where simulation is still initialized with center, size, medium, and then a scence is maybe initialized as a private attribute or is stored as a cached property or something.

@dbochkov-flexcompute
Copy link
Contributor Author

there are currently 4 commits in this PR:

  • "heat solver interface": last review state
  • "heat solver web api": [new stuff] implementation of heat web api based on stub refactor
  • "moving structures/medium back from ...": [new stuff]
    -- removes size and center from Scene and makes them automatically calculated properties.
    -- renames UniformHeatGrid and DistanceHeatGrid to UniformUnstructureGrid and DistanceUnstructureGrid since they could be reused in other solvers
    -- makes AbstractSimulation be a derivative of Box, thus domain field is no longer needed. Simulation domains is set as usual through size and center.
    -- add structures and medium to AbstractSimulation. + property scene + class method from_scene
  • "temporarily limiting matplotlib ...": just a temporary commit to make it easier to deploy and test, can be removed at merge if no longer required

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.

Thanks @dbochkov-flexcompute. I think this looks really good. Mostly this is just minor stuff.

tidy3d/components/base_sim/data/sim_data.py Outdated Show resolved Hide resolved
tidy3d/components/base_sim/data/sim_data.py Outdated Show resolved Hide resolved
tidy3d/components/base_sim/simulation.py Show resolved Hide resolved
tidy3d/components/base_sim/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/bc_placement.py Show resolved Hide resolved
tidy3d/components/scene.py Show resolved Hide resolved
tidy3d/components/simulation.py Show resolved Hide resolved
tidy3d/components/validators.py Outdated Show resolved Hide resolved
tidy3d/web/api/tidy3d_stub.py Show resolved Hide resolved
else:
est_flex_unit = task_info.estFlexUnit
if est_flex_unit is not None and est_flex_unit > 0:
# Wait for task to finish
Copy link
Collaborator

Choose a reason for hiding this comment

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

I won't go through all of this logic, but we should test this pretty thoroughly because the webapi logic tends to break in weird ways sometimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, everything seemed to be working, except for download_log. but it was an issue on backend and now is fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

have you tried job and batch of heat simulation? or even mixes of simulation and heat sim? it could be interesting to see how that works too.

In my experience, sometimes the bugs were happening in web.monitor() in weird edge cases, for example

  • a task errors (if that happens we generally try the task again and see if it errors twice)
  • the timing is somewhat different for whatever reason (task runs before the % done is ready)

There were some improvements earlier this year to fix some of these issues but if this PR changes any of that logic, they could appear again. Will be good to run a bunch just to ensure they don't pop up again because they can be quite tricky to reproduce.

@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/heat-solver branch 2 times, most recently from 6e01916 to 1d63001 Compare September 27, 2023 22:34
@dbochkov-flexcompute
Copy link
Contributor Author

addressed comments + added a beta stage disclaimer in heat solver webapi:

Tidy3D's heat solver is currently in the beta stage. All heat simulations are charged a flat fee of 0.025 FlexCredit.

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

I'm pretty lost here so @dbochkov can you at least help me understand the current state and the plan for the general unification of things?

For example: HeatSimulation inherits from AbstractSimulation, but Simulation does not. Similarly HeatSimulationData inherits from AbstractSimulationData, but SimulationData does not. And while AbstractMonitorData has been introduced, both MonitorData and HeatMonitorData just inherit from Dataset.

Is there currently any duplicated code between Scene and Simulation?

I propose clarifying how everything is meant to work and putting the next few days of effort into actually merging this PR into 2.5 in a way that will avoid duplicated code and further code divergence. Then we can review/fix smaller details.

requirements/basic.txt Outdated Show resolved Hide resolved
@dbochkov-flexcompute
Copy link
Contributor Author

I'm pretty lost here so @dbochkov can you at least help me understand the current state and the plan for the general unification of things?

For example: HeatSimulation inherits from AbstractSimulation, but Simulation does not. Similarly HeatSimulationData inherits from AbstractSimulationData, but SimulationData does not. And while AbstractMonitorData has been introduced, both MonitorData and HeatMonitorData just inherit from Dataset.

Is there currently any duplicated code between Scene and Simulation?

I propose clarifying how everything is meant to work and putting the next few days of effort into actually merging this PR into 2.5 in a way that will avoid duplicated code and further code divergence. Then we can review/fix smaller details.

At this stage HeatMonitorData should inherit from AbstractMonitorData, but MonitorData not. I'll fix that.

So, the idea was to only have heat classes inheriting new Abstract* classes to test the new code organization while keeping optic classes untounched, in case something doesn't work out and we need to rethink it. I planned to do the removal of the duplicated code and switching optic classes to Abstract* classes in a separate PR. Are you suggesting just to go ahead and do that in this PR? I guess that would have the benefit of double-checking that nothing weird happens when proceeding with this unification.

@dbochkov-flexcompute
Copy link
Contributor Author

Otherwise, we seemed to have converged on pretty much everything in this PR. Just need to test out Batch with failing heat tasks

@momchil-flex
Copy link
Collaborator

I think separate PR is fine. Just sooner rather than later. Generally as far as I'm concerned we could merge this soon (after you rebase again and answer all current comments, etc.) And just get it out there so that we can start getting some real user feedback and start fixing things. :)

@momchil-flex
Copy link
Collaborator

momchil-flex commented Sep 30, 2023

Is it currently possible for a user to have a look at the mesh that was used after a solver run? I guess they should at least be able to download the mesh file even if we don't provide any gmesh-wrapped functionality because of their GPL license.

@dbochkov-flexcompute
Copy link
Contributor Author

Is it currently possible for a user to have a look at the mesh that was used after a solver run? I guess they should at least be able to download the mesh file even if we don't provide any gmesh-wrapped functionality because of their GPL license.

yes, it's not ideal currently, but you can directly download mesh ("mesh.cgns") or solution+mesh ("volume_proc0.vtu") using web.core.s3utils.download_file to visualize in thirdparty tools like paraview or pyvista.

This user experience can definitely be improved. Probably, once we have heat monitors that return data on the original unstructured grid instead of interpolated to an equivalent Cartesian one. One idea could be to introduce something like MeshMonitor that only return mesh in the selected region. And if a simulation contains only such monitors, then only meshing is performed and solver step is skipped. This will provide a way to inspect and iterate on mesh before actually solving heat equation. Or just provide meshing-only "solver" similar to mode solver.

@dbochkov-flexcompute dbochkov-flexcompute merged commit 9b11226 into pre/2.5 Oct 6, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants