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

⚡ Charge API & SPICE common types standardization #2106

Closed
wants to merge 33 commits into from

Conversation

daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Dec 5, 2024

Possibly supersedes #2077 if we want to go with this approach?

  • Consolidates Marc's great work in a software structure that I think is more extensible and generic
  • Implement/define consolidated SPICE-compatible element and simulation translation data types
    • Introduce spice DC transfer sources and simulations
    • Start to define basic components (that our monitor_data should be able to compile into).
    • Pending temporal classes
  • Properly integrate RF and CHARGE classes
    • Explored exact integration points and data for the mediums. Proposed a strategy
    • Change the structure of chargespec to use the standard Medium parameters from the frontend
    • Explore a shared stategy for the RF + ChargeMonitorData etc. Basically, whenever we begin doing frequency-dependent or non-static stuff we should then reuse the same monitors as the RF toolsets.
    • Fully fix the validators to share the RF FDTD and TCAD models, depends on backend implementation.
    • Rename classes in a more standard way
      • Cleaner tcad model naming.
      • The ideal situation in terms of defining the equations would be to actually provide a set of serializable model equations using Yannick's technique and dealing with that on the backend, but we can use pre-defined models for now.
  • Add theoretical documentation of what each class is and each parameter. (Determine functional and static data definitions accrodingly)
  • Update all tests to new structure
  • Write proper docs of all classes and how they work (happy to do this depending if we're happy on this approach)

So I will be documenting design decisions here:

  • Ultimately HEAT and CHARGE are TCAD simulation tools and I think it'd be of our benefit to have a well defined tcad directory as it expands so I've reorganised, the way the sofware is structured internally.
  • We need to make sure that our theoretical interfaces are compatible with the other material definitions within our fdtd tools (for the correct frequency range)
  • We need to name the classes independent of their plot/application in order for them to be extensible for future users.
  • Chatting with Damian about how to best share electrical data/models between RF and charge as this ultimately needs to be integrated. Seems like a common SPICE electrical standard seems like it could work.
  • We need to make sure our RF and CHARGE monitors and data are intercompatible, or at least defined with the same class name (and internally check the given data structure)

Things for the future:

  • This could be an unpopular suggestion. I am of the opinion we should use Yannick's serializable strings in order to define the mobility/recombination/etc. models, and enable users to define their own for a given set of parametric properties. However, maybe this is for a future extension and PR
  • The VoltageMonitor that we should should be the same as the RF VoltageMonitor. I've separated them for now since we're only doing electro-static simulations, really.
  • Create SPICE temporal and frequency analysis type definitions - on the lines of these DC ones, possibly that can be co-shared with the RF usage @dmarek-flex just an idea
  • Create the ElectroStaticDataArray -> spice.Capacitor or diode model definition compilation functions, based on the internal parameter declaration

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Dec 5, 2024

Re the RF integration:

I'd like to make a proposal. I am keen to introduce standard SPICE electrical definitions that both RF lumped element classes and the charge tcad API can compile into/abstract from. The problem is that in the current charge implementation, we're basically defining the same electrical elements in slightly different ways and it'd be good to have a shared language at some level. Something on the lines of creating a data type compatible with each component in these spice docs

eg. section 3.3.1:
ngspice resistor definition:

RXXXXXXX n+ n- <resistance|r=>value <ac=val> <m=val>
+ <scale=val> <temp=val> <dtemp=val> <tc1=val> <tc2=val>
+ <noisy=0|1>

so we make an equivalent pydantic data type. Then, this type of class definition can represent both charge ressitors and RF lumped element resistors if it were compiled into spice.

I'm debating whether they should be inherited or whether it would go something on the lines f(LumpedResistor) -> SPICEResistor . If it were inherited, then we'd have a common electrical interface accross the solvers that should in principle make our integration much easier in the future. This also makes a clear boundary between tcad models and rf models in cosimulation.
Still debating how to implement so thought to propose for your thoughts.
So like, for example, one problem that is clear is that Voltage in RF and "potential" in charge ultimately should share some aspect of the the data definition in order to provide clear interconnection in the future.
Maybe this is something we don't necessarily have to solve now, but it'd be good to chat about how we see the solvers being integrated at some point and what data structures are shared and what is separate.
I'm making a draft implementation how the shared data types could look like and maybe we can discuss on top of that?
Yep exactly. Like for every electrical component defined in SPICE including DC voltage source (for example a given device material permittivity might change its RF performance under a DC bias). For example, in heat, the heating element is actually a resistor with some charge through it. In a lumped element case, it might be a 50ohm terminator which might eventually warm up based on some RF power applied onto it. It is concievable we might want to cosimulate this behaviour.

@daquinteroflex daquinteroflex changed the title ⚡ Charge API Consolidation ⚡ Charge API & SPICE common types standardization Dec 6, 2024
@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Dec 8, 2024

Hi @marc-flex , hope you're enjoying the long weekend! So writing this for whenever you get back into this:

What do you think about replacing all these classes with a standard dispersionless Medium definition?

class ChargeSpec(AbstractHeatChargeSpec):
    """Abstract class for Charge specifications"""

    permittivity: float = pd.Field(
        1.0, ge=1.0, title="Permittivity", description="Relative permittivity.", units=PERMITTIVITY
    )


class InsulatorSpec(ChargeSpec):
    """Insulating medium. Conduction simulations will not solve for electric
    potential in a structure that has a medium with this 'electric_spec'.

    Example
    -------
    >>> solid = InsulatingSpec()
    >>> solid2 = InsulatingSpec(permittivity=1.1)

    Note: relative permittivity will be assumed 1 if no value is specified.
    """


class ConductorSpec(ChargeSpec):
    """Conductor medium for conduction simulations.

    Example
    -------
    >>> solid = ConductorSpec(conductivity=3)

    Note: relative permittivity will be assumed 1 if no value is specified.
    """

    conductivity: pd.PositiveFloat = pd.Field(
        1,
        title="Electric conductivity",
        description=f"Electric conductivity of material in units of {CONDUCTIVITY}.",
        units=CONDUCTIVITY,
    )

instead using these standard parameters:

class Medium(AbstractMedium):
    permittivity: TracedFloat = pd.Field(
        1.0, ge=1.0, title="Permittivity", description="Relative permittivity.", units=PERMITTIVITY
    )

    conductivity: TracedFloat = pd.Field(
        0.0,
        title="Conductivity",
        description="Electric conductivity. Defined such that the imaginary part of the complex "
        "permittivity at angular frequency omega is given by conductivity/omega.",
        units=CONDUCTIVITY,
    )

I need to check the backend PR more closely, but if we do need to identify that a Medium is a conductor or insulator I have the impression we should be able to determine this from the defined parameters? This way we have direct integration with the RF material definitions accordingly. This means that the electric_spec could be broadly removed and determined from the FDTD medium models? I'm going to go ahead with this as think it should be broadly reasonable, but if not we can bring it back. If we need to split models based on the frequency dependence permittivity, we'll explore that I think then.

@daquinteroflex daquinteroflex added the 2.8 will go into version 2.8.* label Dec 8, 2024
@daquinteroflex
Copy link
Collaborator Author

Hi @marc-flex , so the PR is still dirty in the sense that it needs polishing and some final tasks that we can split between ourselves (but some depend on the backend implementation).

It's a relatively large refactor of the tcad tools structure that in principle should be compatible to develop at least, has defined integration points for the RF cosimulation for the future, and a more clean data/software structure imo.

I've written in the code the TODOs that maybe we need to decide how to polish together since it's backend dependent. I believe the conceptual data should be pretty much identical to before, just that more closely integrated with FDTD models possibly.

We don't have to use this structure, and am open to doing it differently if we can come up with a better approach - but it's an attempt at least of how this tools makes sense to me both as a user and to extend. Happy to meet up via slack and chat if we want after you have a look.

@dmarek-flex
Copy link
Contributor

What do you think about replacing all these classes with a standard dispersionless Medium definition?

I think I understand why @marc-flex did it this way, and it might be helpful to have the same or similar approach for the RF simulations. Being able to distinguish between an insulator or conductor is useful for helping to set up a valid Simulation. For one, it might make it easier to automatically apply LossyMetalMedium type to structures that would benefit. It could also be useful in a future validation step where we check that a LumpedPort is electrically connected to 2 conductors, which is required for a valid simulation. Another example is finding out which structures intersecting a plane are good conductors, which would help automatically setup path integrals for the WavePort.

I am not sure if it is better to make a user make that distinction or by checking the value of the conductivity automatically.

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Dec 9, 2024

Nice, yeah that makes sense.

Would there be a reason we can't computationally distinguish a given medium model being an insulator or conductor based on, say, it's electrical conductivity parameter from the standard EM medium library above a given threshold? On this original implementation, the user would have to redefine the ChargeSpec on top of the EM medium spec if they were to do cosimulation - which can be tedious. From a user perspective, the easiest way to use it would be to define the broad material in the given structure and then let the tools manage the RF and CHARGE behavior on the backed automatically. It sounds like ultimately we want to consolidate these RF medium definitions and the charge ones anyway as they are within the same frequency domain for the physical parameters we care about.

The best would be to just use the existing Medium definitions (since we can leverage the existing materials library and users don't have to redefine another spec for the same class that contains broadly the same information as the rf fdtd regime). If we need to distinguish between what electronic/electric model to apply, I am of the opinion that rather than creating a separate class, we should just make a method within the abstract Medium model class that basically computes in what regime the medium should be treated based the definedmaterial parameters. This method gets overwritten based on the medium model (if in the future we want to support say anisotropic co-simulations etc). Based on which medium models we primarily will use in the RF simulations, we can determine whether we should treat them as insulators or conductors on the backend possibly? However if we need and the above approach would not work for some reason, we can still create an InsulatorSpec, ConductorSpec, or LossyMetalMedium as originally intended within the high-level medium definitions with no problem (bringing that back is easier than reconsolidating likely anyway).

Just ideas, you know better than me how to best use this on the backend so we can keep it as it was originally with no problem.

@daquinteroflex
Copy link
Collaborator Author

So worth mentioning this is just a proposal structure anyways but these maybe open the questions we want to decide on anyways. We'll have to deal with cosimulation eventually and maybe it'd be good to plan this so we don't have to rewrite the structure/strategy in a way that affects users after we release.

Ultimatey the questions relate to shared simulation parameter definitions and shared output data structures.

It sounds like right now charge would be electrostatic but some of the parameters can be extended to be dynamic. And deciding between separate or shared monitors/dataarray accordingly also in terms of what is the easiest usage for the end-user creating these simulations.

Maybe @momchil-flex @dbochkov-flexcompute maybe you have some thoughts on how we could approach/structure this

Comment on lines +1196 to +1221
class ElectroStaticDataArray(DataArray):
# TODO proper docs Given the combinatorial amount of variations of electro-static performance parameters,
# I think it makes sense to have a single data array that can handle these type of parameter sweeps, and we just
# check for them. Note we need to validate the data aligns properly between the swept variaable. It might be the
# case C-I-V all get swept at the same time
"""
Semiconductor I-V curve data array.

Example
-------
>>> I = [0. 0, 1, 4]
>>> V = [-1, -0.5, 0, 0.5]
>>> td = ElectroStaticDataArray(data=I, coords={"Voltage (V)": V})

Semiconductor DC capacitance variations with respect to voltage.

Example
-------
>>> C = [0. 0, 1, 4]
>>> V = [-1, -0.5, 0, 0.5]
>>> td = ElectroStaticDataArray(data=C, coords={"Voltage (V)": V})
"""

__slots__ = ()
_dims = ("Voltage (V)",)
_data_attrs = {"long_name": "Current (A)"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The name has been generalized but this will print everything as I-V curve since _data_attrs = {"long_name": "Current (A)"}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reckon we keep it as before but maybe think how to implement this more generally in the future.

Comment on lines +1354 to +1359
# TODO Replace with standard medium info.
# elif property == "electric_conductivity":
# medium_list = [
# medium for medium in medium_list if isinstance(medium.electric_spec, ConductorSpec)
# ]
# cond_list = [medium.electric_spec.conductivity for medium in medium_list]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there electric conductivity data in medium?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea to have a common place to store common structures but I don't like the name SPICE 😂 it kind of says we're integrating the open-source tool

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, it might be misleading. could it be as simple as "components/circuit"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good point. However, SPICE is just one type of standard of analogue/mixed-signal electrical language common to multiple tools https://en.wikipedia.org/wiki/SPICE#Successors so it does make sense to relate to it (I'm not super sure there's a comprehensive electrical modelling alternative in order to generalise further? Other tools specifically refer to SPICE too and it's directly useful and very common for users to require this tool abstraction.

Would be keen to retain this specific directory if that sounds good since ultimately it's our abstraction of the SPICE standard language?

Comment on lines +19 to +22
class TransferFunctionDC(Tidy3dBaseModel):
"""This class sets parameters used in DC simulations.

Ultimately, equivalent to Section 11.3.2 in the ngspice manual.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this name. It sort of implies we get the response of a device/circuit for a range of input values (e.g., voltage) which may not be the case? We could be interested in a single point operating point?

Comment on lines +38 to +61
sources: StaticTransferSourceDC | MultiStaticTransferSourceDC = []

absolute_tolerance: Optional[pd.PositiveFloat] = pd.Field(
default=1e10,
title="Absolute tolerance.",
description="Absolute tolerance used as stop criteria when converging towards a solution. Should be equivalent to the"
"SPICE ABSTOL DC transfer parameter. TODO MARC units, TODO check equivalence. TODO what does this mean?",
)

relative_tolerance: Optional[pd.PositiveFloat] = pd.Field(
default=1e-10,
title="Relative tolerance.",
description="Relative tolerance used as stop criteria when converging towards a solution. Should be equivalent to the"
"SPICE RELTOL DC transfer parameter. TODO MARC units, TODO check equivalence. TODO what does this "
"mean?",
)

dc_iteration_limit: Optional[pd.PositiveInt] = pd.Field(
default=30,
title="Maximum number of iterations.",
description="Indicates the maximum number of iterations to be run. "
"The solver will stop either when this maximum of iterations is met "
"or when the tolerance criteria has been met. Should be equivalent to the ngspice ITL1 parameter. TODO units, "
"TODO devsim ngspice equivalence",
Copy link
Contributor

Choose a reason for hiding this comment

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

Another point here is that we're defining here tolerances for this DC case. This should be defined somewhere else since we'll need them for he transient cases as well.

Also, I find it a bit too verbose, which may be annoying from a user standpoint. Like we use heat_spec as opposed to heat_specification to make code more compact. Of course, this is a matter of preference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep I think we follow the standard tolerance setting on ngspice for the transient configuration then which will be defined.

Hahaha bringing back the good old question:

I do incline for clear understandability of variables even if they're long normally because it's easier to know what something is without reading the documentation but yeah it's a matter of preference. We can have both in pydantic normally but I do appreciate it's a bit more annoying.

Hmm will have a think how to make this better.

Comment on lines +29 to +44
class AbstractStaticTransferSourceDC(Tidy3dBaseModel):
name: str
start: Optional[pd.PositiveFloat] = 0
stop: Optional[pd.PositiveFloat] | None = None
"""
TODOMARC chat Either they define the stop or they define the step, how do we want to enforce, validator?
"""

step: Optional[pd.PositiveFloat] = pd.Field(
1.0,
title="Bias step.",
description="By default, a solution is computed at 0 bias. "
"If a bias different than 0 is requested, DEVSIM will start at 0 and increase bias "
"at 'dV' intervals until the required bias is reached. ",
)
# TODO units
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how you organized this. It makes a lot of sense now that I see it!
I do have a concern, though: the way you'd define a simulation here would differ from the way sources are defined in FDTD and HEAT, wouldn't it? The source would be inside a TransferFunctionDC and not at the simulation object level, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I believe as we enter more electronic simulations, the definition of what a simulation is will evolve since they can require a combination of DC stimuli and RF stimuli together. As such, we need a way to discretise the relevant simulation steps and combine analysis in flexible ways. This is different from the frequency domain FDTD method so I do think this makes sense to extend though.

On the way the sources get integrated, I am of the opinion in this case it does make sense because of the 1D parametric nature of this source that - still - might directly interact with other sources from other solves. Towards where we need to go I think this structure makes sense and can be integrated in the normal flow.

Copy link
Contributor

@marc-flex marc-flex Dec 16, 2024

Choose a reason for hiding this comment

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

I don't think the simulation object necessarily needs to evolve for this but maybe we need to think about having a workflow object with which we can define some sort of (loosely) coupled simulations (charge and heat can and should/will be fully coupled but I don't think we'll ever have fully coupled charge-RF or Charge-FDTD). What do you think? @momchil-flex @dbochkov-flexcompute

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sounds good! We could keep the current ***Simulation definitions while combining them in some sort of super-class later on. We can revisit this but to be fair our initial discussions about having a single Simulation-like class that can contain all sorts of components (e.g. optical, heat, charge sources) and then trying to parse what to do internally based on that always sounded to me a bit too ambitious and potentially still not ideal even if we did manage to handle it well, because again it would be hard for the user to have a mental map of what exactly is happening (i.e. how our "translational layer" works). I think similarly to what we discussed about MultiPhysicsMedium, having the separate media that go into different solvers explicitly defined is a bit more cumbersome to initialize, but at least much more explicit from the user point of view. Similarly I would probably imagine us heading to a MultiPhysicsSimulation where simulations associated to various solvers are defined, as well as some specification about how they couple - even if this will not be the most concise way to do things. This could be the same as the Workflow object @marc-flex mentions, or more likely there could be a translation from a MultiPhysicsSimulation to a Workflow that explicitly shows the order of solves that will be done and how e.g. one monitor becomes a medium or a source in another simulation, so the user can exmaine it in advance.

Again, I'm sure we'll be iterating on this a lot but it's probably what I'm leaning towards after our recent discussions.

Comment on lines +10 to +11
class HeatChargeBC(ABC, Tidy3dBaseModel):
"""Abstract heat-charge boundary conditions."""
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to change the directory we might as well change the naming here to TcadBC or something along those lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@momchil-flex what do you think about this directory name? Is it ok?

We can still make things like the HeatSimulation backwards compatible by just changing the import location at the tidy3d top level and making sure no internals change at all in the updated PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure. On one hand I feel like TCAD is a bit too general here. Also when you think what it means, not referring to any specific physics but just to computer-aided design, it seems strange to call things e.g. TcadBC. On the other hand, it is widely used and understood to be centered around exactly what we're solving here. Maybe my suggestion would be to rename the folder while still keeping HeatCharge in component names? Does that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep nice, I agree

Comment on lines +9 to +26
class VoltageBC(HeatChargeBC):
"""Electric potential (voltage) boundary condition.
Sets a potential at the specified boundary.
In charge simulations it also accepts an array of voltages.
In this case, a solution for each of these voltages will
be computed.

Example
-------
>>> bc1 = VoltageBC(voltage=2)
>>> bc2 = VoltageBC(voltage=[-1, 0, 1])
"""

voltage: Union[pd.FiniteFloat, Tuple[pd.FiniteFloat, ...]] = pd.Field(
title="Voltage",
description="Electric potential to be applied at the specified boundary.",
units=VOLT,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this now derive from AbstractStaticTransferSourceDC or I misunderstood?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the intention to use BCs to apply the bias voltage?

Copy link
Collaborator Author

@daquinteroflex daquinteroflex Dec 14, 2024

Choose a reason for hiding this comment

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

I was thinking about this more today.

Should we have the VoltageBC also work as an iterative parameter? Part of me thinks the proper setup would be to create a boundary condition from a DC source rather than iterate directly from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm not following. So far this is always DC. Maybe we can have a chat? I we might be thinking about different things? 😅

Comment on lines +71 to +72
class StaticChargeCarrierData(HeatChargeMonitorData):
"""Class that stores free carrier concentration in Charge simulations."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe DCFreeCarrierData?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One reason why the static naming maybe makes sense is due to the temporal nature of our simulations, which when they are multi-physical then this changes. For example, free carriers may change on a static heat simulation, independent of a DC voltage/current. As such static represents this temporal relationship clearly I think.

Comment on lines +165 to +166
class StaticVoltageData(HeatChargeMonitorData):
"""Data associated with a :class:`VoltageMonitor`: spatial electric potential field.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about ElectrostaticPotentialData? Also, with minimum modifications this one and the free carrier one can be reused for harmonic response so they would not longer be DC or static. Having said that, maybe derived classes could also be defined

Comment on lines +22 to +25
class ElectronicSpec(AbstractHeatChargeSpec):
# TODO does this account for the whole temporal dynamics? If not then it should be called ElectroStatic, which it was called before, but unsure why it got changed?
"""
This class is used to define electro-static semiconductors.
Copy link
Contributor

Choose a reason for hiding this comment

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

This was called SemiconductorSpec because it only makes sense for semiconductors. Temperature is included, not only in the different models but, more importantly, in the drift-diffusion equations, which consider Boltzmann statistics

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that makes sense.

The issue I have with the definition of SemiConductor is that this does not imply the "active" nature of a material. Si, Si02 are all semiconductors, but they can also be treated as insulators, or even good conductors, depending on the simulation conditions ie optical/electrical. Electronic is probably not the answer you are right. "ActiveSemiConductorMedium" maybe sounds good to me.

Maybe we can chat about this in #2124

Comment on lines +33 to +41
electrons_effective_density: pd.PositiveFloat = pd.Field(
2.86e19,
title="Effective density of electron states",
description="Effective density of electron states",
units="cm^(-3)",
)
N_e = electrons_effective_density

holes_effective_density: pd.PositiveFloat = pd.Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

These names are confusing. The literature universally calls this effective density of states with Nc and Nv as symbols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Broadly inclined to bring back your standard naming on this and maybe just make the description longer explaining it more.

Comment on lines +81 to +86
exponent_temperature_reference_doping: float = pd.Field(
2.4,
title="Exponent of thermal dependence of reference doping.",
description="Exponent of thermal dependence of reference doping.",
)
exp_t_d = exponent_temperature_reference_doping
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I don't like about verbose names: you end up needing to duplicate variables with shorter names otherwise equations become 100 lines. On top of that, you already have title and description to help users. I personally find these counterproductive. Just my opinion though 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't really have a well-established guideline about this. I wouldn't use exp_t_d but I do see the point that exponent_temperature_reference_doping is super long. Something like exp_temp_ref_doping seems like a middle ground that sounds OK to me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe in this very specific model cases it makes sense to have it as it was originally, if we can provide a mathematical description of the equation in the documentation of the class. I do tend for verbosity because that way I don't have to check extra comments or documentation on a variable, but in this case that doesn't make sense as it is a mathematical model and it is required to check it anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a latex formula in the description? That would make this a bit easier to follow

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think something like this should work

:math:`\\alpha = \\alpha_0 + \\beta I`

This is from our two photon absorption spec, it appears in the main docstring but I'm pretty sure the descriptions are parsed in the same way. Alternatively you can also add a Note, you can also refer to that class: https://docs.flexcompute.com/projects/tidy3d/en/latest/_modules/tidy3d/components/medium.html#TwoPhotonAbsorption

@@ -0,0 +1,18 @@
from typing import Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand how I could reuse everything in the components folder, but for the RF solver I don't see how I would make use of the classes in analysis or sources. At some point if we support cosimulation with spice it would make more sense, but at the moment I think the main operation would be to extract S parameters from device using Tidy3D and then using the S parameter model of the device in a larger circuit.

Copy link
Contributor

@marc-flex marc-flex left a comment

Choose a reason for hiding this comment

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

Thanks @daquinteroflex for having looked at this. I think this was needed. A common interface between RF and charge is definitely required and this provides just that!

Comment on lines +544 to +549
for structure in structures:
if isinstance(structure.medium.electric_spec, ElectronicSpec):
if (
structure.medium.electric_spec.donors is not None
or structure.medium.electric_spec.acceptors is not None
):
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you had removed electric_spec from Medium?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I've set the foundations of the new structure in #2124 but I think we need to have a discussion how we want to manage the new approach based on that potential API

@momchil-flex
Copy link
Collaborator

If this is merged, would it make the HeatSolver not backwards compatible?

@daquinteroflex
Copy link
Collaborator Author

So made a plan with Marc on how we'll implement this in stages so will close this PR for now. Some of this stuff will be reused with your feedback in a different PR likely. It'd still be great to discuss the feedack points as some of these things would be relevant for the future.

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.* rc2 2nd pre-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants