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

Enabling electrostatic simulations #1561

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Enabling electrostatic simulations #1561

merged 1 commit into from
Jul 3, 2024

Conversation

marc-flex
Copy link
Contributor

No description provided.

@dbochkov-flexcompute
Copy link
Contributor

  • I think we will have to iterate on naming a few times. My current suggestion would be:
    semiconductor_dev/ -> device/
    semicon_dev_spec -> charge_spec
    AbstractSemiConDevSpec -> AbstractChargeSpec
    InsulatingSpec -> InsulatorSpec
    DeviceSpec -> ConductorSpec
    PotentialBC -> other options could be BiasBC and VoltageBC
    ElectricBoundarySpec -> maybe more explicit PotentialBoundarySpec or ElectricPotentialBoundarySpec
    UniformChargeSource -> UniformChargeDistribution
    ElectrostaticSimulation -> DeviceSimulation or ChargeSimulation
    ElectrostaticSimulationData -> DeviceSimulationData or ChargeSimulationData
  • Note that this PR should be focusing on solving grad(sigma * grad(phi)) = 0 for conductors, rather than grad(eps * grad(phi) = rho for dielectrics (we will need that too, but later)
  • Thus, DeviceSpec/ConductorSpec should contain conductivity instead of permittivity
  • I see that you decided not to implement conversion back and forth on the backend and did this inside webapi. Do you plan to keep it this way?
  • A bigger question is the overall organization of the different solver classes. Currently ElectrostaticSimulation and related classes duplicate a lot of code from HeatSimulation and its related classes. We should definitely try to avoid that. I see two ways:
  1. introduce an intermediate class UnstructuredSimulation(AbstractSimulation) that will absorb the duplicated functionality, and so that HeatSimulation(UnstructuredSimulation) and ChargeSimulation(UnstructuredSimulation). Similar for other classes.
  2. start unifying heat and charge simulation in a single class DeviceSimulation. Meaning that depending on how you setup the simulation it will either calculate for heat, for charge, or for both.

Personally I am leaning towards the second path. But we can iron that out offline.

@marc-flex
Copy link
Contributor Author

  • I see that you decided not to implement conversion back and forth on the backend and did this inside webapi. Do you plan to keep it this way?

Didn't have an idea in mind and doing it on the webapi seemed easy. I can try doing it on the backend. This possibly makes more sense since I'm going to refactor everything.

2. start unifying heat and charge simulation in a single class DeviceSimulation. Meaning that depending on how you setup the simulation it will either calculate for heat, for charge, or for both.

I think I prefer this option too. I will start form here and tailor the rest towards this. This will also avoid code duplication, I wasn't really happy with the version I had but I wanted a starting point.

@tylerflex
Copy link
Collaborator

Is it ok for me to convert this PR a "draft" or is it officially ready for review?

image

(note, we just tend to do this for PRs in progress to make it easier to see which PRs are currently awaiting review / merge)

@dbochkov-flexcompute dbochkov-flexcompute marked this pull request as draft April 10, 2024 04:51
@dbochkov-flexcompute
Copy link
Contributor

Is it ok for me to convert this PR a "draft" or is it officially ready for review?

image (note, we just tend to do this for PRs in progress to make it easier to see which PRs are currently awaiting review / merge)

Yeah, this is still in the draft state

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

Nice work! I think it's getting close. Some general comments:

  • this is not currently allowed, but for when we have coupled current/heat simulation, what do you think about plotting boundary conditions for different equations? because those can be defined on the same boundaries. Maybe just display either only heat or only electrical bc depending on plot arguments?
  • All new classes/components and different ways of using them will need to be covered by tests

tidy3d/__init__.py Outdated Show resolved Hide resolved
tidy3d/components/device/boundary.py Outdated Show resolved Hide resolved
tidy3d/components/device_spec.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/components/scene.py Outdated Show resolved Hide resolved
tidy3d/components/device/source.py Outdated Show resolved Hide resolved
tidy3d/components/device/source.py Outdated Show resolved Hide resolved
tidy3d/components/device/source.py Outdated Show resolved Hide resolved
tidy3d/components/device/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/device/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/device/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/device/boundary.py Outdated Show resolved Hide resolved
tidy3d/components/device/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/device/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/device/simulation.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/components/device/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/device/source.py Outdated Show resolved Hide resolved
@marc-flex marc-flex requested a review from tylerflex May 15, 2024 09:37
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 @marc-flex, this is a really huge PR and I really appreciate all of the hard work you put into it.

I think the biggest question is whether we want to name this stuff Device simulation. I made a comment maybe we can discuss in a thread.

At a high level, if we can simplify any of the internals by using properties or defining things in the class datastructures, that will help us in the long run to cut down on the code complexity. I made comments where I noticed things.

Overall it looks quite good, thanks again!

tests/test_components/test_device.py Outdated Show resolved Hide resolved
tests/test_components/test_device.py Outdated Show resolved Hide resolved
tests/test_components/test_device.py Outdated Show resolved Hide resolved
tests/test_components/test_device.py Outdated Show resolved Hide resolved
tidy3d/components/device/boundary.py Outdated Show resolved Hide resolved
tidy3d/components/device/source.py Outdated Show resolved Hide resolved
tidy3d/components/device/source.py Outdated Show resolved Hide resolved
tidy3d/components/device_spec.py Outdated Show resolved Hide resolved
tidy3d/components/device_spec.py Outdated Show resolved Hide resolved
tidy3d/web/api/webapi.py Outdated Show resolved Hide resolved
@tylerflex tylerflex added the 2.8 will go into version 2.8.* label May 22, 2024
@dbochkov-flexcompute dbochkov-flexcompute marked this pull request as ready for review May 28, 2024 22:29
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

added a few more comments after the big renaming

tidy3d/components/heat_charge/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/heat_charge/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/heat_charge/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/heat_charge/simulation.py Outdated Show resolved Hide resolved
Comment on lines +594 to 645
boundaries = self._construct_heat_charge_boundaries(
structures=structures,
plane=plane,
boundary_spec=self.boundary_spec,
Copy link
Contributor

Choose a reason for hiding this comment

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

will this plot heat and condution bc at the same time? That's probably will not be very useful as they will overlap with each other generally. Probably we should add a parameter in plot_boundaries which tells whether to plot heat or charge bcs, so that when plot_property is called, only corresponding bcs are plotted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking of changing all the features plot based on property so that if property="heat_conductivity", sources, bcs and monitors will be plotted for the HEAT setup, whereas it property="electric_conductivity" then all features will be CONDUCTION.

tidy3d/components/heat_charge/simulation.py Outdated Show resolved Hide resolved
Comment on lines 1169 to 1170
"Electric BC were specified but not all "
"structures have a '.medium' with `.electric_spec` present"
Copy link
Contributor

Choose a reason for hiding this comment

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

are we checking here for all structures to have electric_spec or just at least one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it checks that at least one has electric_spec

tidy3d/components/heat_charge/source.py Outdated Show resolved Hide resolved
tidy3d/components/heat_charge/source.py Outdated Show resolved Hide resolved
tidy3d/web/api/webapi.py Outdated Show resolved Hide resolved
@@ -128,22 +128,27 @@ def plot_field(
"""

monitor_data = self[monitor_name]
property_to_plot = None

if isinstance(monitor_data, TemperatureData):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks really interesting! I guess in the future we'll be able to also plot any specific overlapped relationships between the monitors too at the field level, although maybe this would be user-specific rather than provided possibly

Comment on lines 67 to 77
Coupling between these simulations is currently limited to 1-way coupling between
heat and conduction simulations. Coupling is specified by defining a heat source of
type 'HeatFromElectricSource'. With this coupling, joule heating is calculated as part
of the solution to a CONDUCTION simulation and then read in to the HEAT simulation.
When using coupling we anticipate two scenarios:
1. one in which BCs and sources are specified for both HEAT and CONDUCTION simulations.
In this case one mesh will be generated and used for both the CONDUCTION and HEAT
simulations.
2. only heat BCs/sources are provided. In this case, only the HEAT equation will be solved.
Before the simulation starts, it will try to load the heat source from file so a
previously run CONDUCTION simulations must have run previously. Since the CONDUCTION
Copy link
Collaborator

@daquinteroflex daquinteroflex Jun 12, 2024

Choose a reason for hiding this comment

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

Just future improvement ideas: maybe we can make a little diagram to just illustrate this for people scrolling through the docs. Happy to help when I get some time as I've got the lucidchart available, or can share too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea! I'll try to prepare something, thought maybe for another PR?

Copy link
Collaborator

@daquinteroflex daquinteroflex left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for such an incredible effort! Only API docs implementations are missing in the PR I believe.

Lots to think about of what this this will enable! It'd be interesting to do a demo of a standard basic CMOS electronic device (eg. MOScapacitor, etc) for that crowd too to some level - especially for parametric design.

@marc-flex
Copy link
Contributor Author

Lots to think about of what this this will enable! It'd be interesting to do a demo of a standard basic CMOS electronic device (eg. MOScapacitor, etc) for that crowd too to some level - especially for parametric design.

@daquinteroflex If you have an example/paper I'll be happy to work on a notebook!

@marc-flex marc-flex changed the base branch from pre/2.7 to develop June 14, 2024 09:26
@tylerflex tylerflex self-requested a review June 19, 2024 15:43
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.

Make sure to update the CHANGELOG.md as well! including all of the relevant changes that a user might need to know. Thanks!

@marc-flex marc-flex changed the base branch from develop to pre/2.8 June 21, 2024 18:02
@tylerflex
Copy link
Collaborator

Hey @marc-flex when this looks all ready to merge, let me know and I will merge into pre/2.8! or you can do it if you have access.
image

Added test for HeatChargeSimulation
Added functionality to compute heat source from conduction simulation
@marc-flex marc-flex merged commit 8869458 into pre/2.8 Jul 3, 2024
16 checks passed
@marc-flex marc-flex deleted the marc/electrostatic branch July 3, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 will go into version 2.8.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants