-
Notifications
You must be signed in to change notification settings - Fork 66
feat(FXC-1655): Add SSAC voltage source #2808
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing changes made in this pull request
Notes | ||
----- | ||
- The bias `voltage` refer to the DC operating point above the simulation ground. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be RST syntax here
``voltage``
As a rule, we don't use markdown notation in the docs. It's either RST (two "`"-s) in docstrings, or single quotes '
in warning/error messages that will be printed to stdout.
voltage: ArrayFloat1D = pd.Field( | ||
..., | ||
title="DC Bias Voltages", | ||
description="List of DC operating point voltages (above ground) used with 'VoltageBC'.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also double ` here as it gets converted to a docstring.
----- | ||
- The bias `voltage` refer to the DC operating point above the simulation ground. | ||
- The `frequency` array lists the AC analysis frequencies (in Hz). | ||
- The `amplitude` is the small-signal AC magnitude (in Volts). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the "frequency" and "amplitude" Note are not really needed given that the information is also given in the field description.
frequency: ArrayFloat1D = pd.Field( | ||
..., | ||
title="AC Analysis Frequencies", | ||
description="List of small-signal analysis frequencies (in Hz).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ac_current_voltage: Optional[FreqPotentialDataArray] = pd.Field( | ||
None, | ||
title="AC current vs voltage and frequency", | ||
description="Complex small-signal current I(v, f) as a FreqPotentialDataArray.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no need to say as a FreqPotentialDataArray
here. Since the field type is Optional[FreqPotentialDataArray]
, this information will already appear in the parsed docstring, including a clickable link to the type.
bc.condition.source.amplitude, | ||
) | ||
raise SetupError( | ||
"'HeatSimulation' does not include any `SSACVoltageSource` in 'boundary_spec')." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
' everywhere in error messages.
@pd.validator("amplitude") | ||
def validate_amplitude(cls, val): | ||
if val == td_inf: | ||
raise ValueError(f"Small signal amplitude must be finite. Current amplitude={val}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud here, we want the amplitude to not just be finite, but "small" in some sense. Is there something that we can test it against and at least issue a warning if it doesn't seem to be the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this type of analysis the perturbed output varies linearly (i.e., perturbation n, p, phi) with the amplitude so in principle in doesn't matter the amplitude. A different story is whether the solution is physically meaningful. I think it is, in general, not straightforward to define a validity limit and it may indeed be the case that it is case-dependent.
It won't hurt, however, to include some warning or note in the docstring mentioning the validity of the simulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @damiano-flex
I left a few comments but this is looking good!
I guess a more general comment, Do we want to include field output? Such as maybe perturbation current, perturbation potential etc?
@pd.validator("amplitude") | ||
def validate_amplitude(cls, val): | ||
if val == td_inf: | ||
raise ValueError(f"Small signal amplitude must be finite. Current amplitude={val}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this type of analysis the perturbed output varies linearly (i.e., perturbation n, p, phi) with the amplitude so in principle in doesn't matter the amplitude. A different story is whether the solution is physically meaningful. I think it is, in general, not straightforward to define a validity limit and it may indeed be the case that it is case-dependent.
It won't hurt, however, to include some warning or note in the docstring mentioning the validity of the simulation.
tidy3d/components/data/data_array.py
Outdated
class FreqPotentialDataArray(DataArray): | ||
"""Frequency-domain array. | ||
Example | ||
------- | ||
>>> f = [2e14, 3e14] | ||
>>> v = [0.1, 0.2, 0.3] | ||
>>> coords = dict(f=f, v=v) | ||
>>> fd = FreqPotentialDataArray((1+1j) * np.random.random((2, 3)), coords=coords) | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want FreqPotentialDataArray
or FreqVoltageDataArray
? I think it'd be more consistent with the other classes with voltage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely remember this was already introduced for RF hmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fyi
tidy3d/tidy3d/components/data/data_array.py
Lines 1363 to 1375 in 6e1ff22
class VoltageFreqDataArray(VoltageArray, FreqDataArray): | |
"""Voltage data array in frequency domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9, 4e9] | |
>>> coords = dict(f=f) | |
>>> data = np.random.random(3) + 1j * np.random.random(3) | |
>>> vfd = VoltageFreqDataArray(data, coords=coords) | |
""" | |
__slots__ = () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was all recently added fyi
tidy3d/tidy3d/components/data/data_array.py
Lines 1344 to 1500 in 6e1ff22
class VoltageArray(DataArray): | |
# Always set __slots__ = () to avoid xarray warnings | |
__slots__ = () | |
_data_attrs = {"units": VOLT, "long_name": "voltage"} | |
class CurrentArray(DataArray): | |
# Always set __slots__ = () to avoid xarray warnings | |
__slots__ = () | |
_data_attrs = {"units": AMP, "long_name": "current"} | |
class ImpedanceArray(DataArray): | |
# Always set __slots__ = () to avoid xarray warnings | |
__slots__ = () | |
_data_attrs = {"units": OHM, "long_name": "impedance"} | |
# Voltage arrays | |
class VoltageFreqDataArray(VoltageArray, FreqDataArray): | |
"""Voltage data array in frequency domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9, 4e9] | |
>>> coords = dict(f=f) | |
>>> data = np.random.random(3) + 1j * np.random.random(3) | |
>>> vfd = VoltageFreqDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
class VoltageTimeDataArray(VoltageArray, TimeDataArray): | |
"""Voltage data array in time domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> t = [0, 1e-9, 2e-9, 3e-9] | |
>>> coords = dict(t=t) | |
>>> data = np.sin(2 * np.pi * 1e9 * np.array(t)) | |
>>> vtd = VoltageTimeDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
class VoltageFreqModeDataArray(VoltageArray, FreqModeDataArray): | |
"""Voltage data array in frequency-mode domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9] | |
>>> mode_index = [0, 1] | |
>>> coords = dict(f=f, mode_index=mode_index) | |
>>> data = np.random.random((2, 2)) + 1j * np.random.random((2, 2)) | |
>>> vfmd = VoltageFreqModeDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
# Current arrays | |
class CurrentFreqDataArray(CurrentArray, FreqDataArray): | |
"""Current data array in frequency domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9, 4e9] | |
>>> coords = dict(f=f) | |
>>> data = np.random.random(3) + 1j * np.random.random(3) | |
>>> cfd = CurrentFreqDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
class CurrentTimeDataArray(CurrentArray, TimeDataArray): | |
"""Current data array in time domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> t = [0, 1e-9, 2e-9, 3e-9] | |
>>> coords = dict(t=t) | |
>>> data = np.cos(2 * np.pi * 1e9 * np.array(t)) | |
>>> ctd = CurrentTimeDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
class CurrentFreqModeDataArray(CurrentArray, FreqModeDataArray): | |
"""Current data array in frequency-mode domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9] | |
>>> mode_index = [0, 1] | |
>>> coords = dict(f=f, mode_index=mode_index) | |
>>> data = np.random.random((2, 2)) + 1j * np.random.random((2, 2)) | |
>>> cfmd = CurrentFreqModeDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
# Impedance arrays | |
class ImpedanceFreqDataArray(ImpedanceArray, FreqDataArray): | |
"""Impedance data array in frequency domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9, 4e9] | |
>>> coords = dict(f=f) | |
>>> data = 50.0 + 1j * np.random.random(3) | |
>>> zfd = ImpedanceFreqDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
class ImpedanceTimeDataArray(ImpedanceArray, TimeDataArray): | |
"""Impedance data array in time domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> t = [0, 1e-9, 2e-9, 3e-9] | |
>>> coords = dict(t=t) | |
>>> data = 50.0 * np.ones_like(t) | |
>>> ztd = ImpedanceTimeDataArray(data, coords=coords) | |
""" | |
__slots__ = () | |
class ImpedanceFreqModeDataArray(ImpedanceArray, FreqModeDataArray): | |
"""Impedance data array in frequency-mode domain. | |
Example | |
------- | |
>>> import numpy as np | |
>>> f = [2e9, 3e9] | |
>>> mode_index = [0, 1] | |
>>> coords = dict(f=f, mode_index=mode_index) | |
>>> data = 50.0 + 10.0 * np.random.random((2, 2)) | |
>>> zfmd = ImpedanceFreqModeDataArray(data, coords=coords) | |
""" | |
__slots__ = () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @daquinteroflex!
Last time I spoke to @dmarek-flex we kind of reserved "voltage" to whenever we're comparing potentials and "potential" when we talk about the field. But maybe @dmarek-flex can confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, same units but conceptually fairly different. Not a strong feeling, but I do think that FreqPotentialDataArray
is a better description that also differentiates it more from the VoltageArray
s we introduced in RF.
Sadly if we don't modify we will have FreqVoltageDataArray
and VoltageFreqDataArray
😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still don't have an enforced naming scheme for DataArray
, but one idea is to have the leading term prefix relate to the quantity (Voltage/Current/Potential) and the remaining terms relate to the coordinates (Freq/Mode/Spatial). So maybe follow that approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I might be confused about the purpose of FreqVoltageDataArray
. The coordinates are frequency and voltage? In that case, I agree with the current approach 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, coordinates are frequency and voltage.
This source represents a small-signal AC excitation defined by a list of bias voltages, | ||
a set of small-signal analysis frequencies, and a small-signal amplitude (linear scale). | ||
The bias ``voltage`` refer to the DC operating point above the simulation ground. Currently, electrical ports are not defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refer
-> refers- We don't really define a ground, do we? We define a voltage difference. Or have we changed this for SSAC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had a discussion with @daquinteroflex about this. We need to discuss about on how and where let the user set the ground.
bc.condition.source.amplitude, | ||
) | ||
raise SetupError( | ||
"'HeatSimulation' does not include any 'SSACVoltageSource' in 'boundary_spec')." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HeatSimulation
is deprecated. We should use HeatChargeSimulation
instead
def _get_voltage_regime(self): | ||
for bc in self.boundary_spec: | ||
if isinstance(bc.condition, VoltageBC): | ||
if isinstance(bc.condition.source, SSACVoltageSource): | ||
return "ac" | ||
return "dc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could generalize this to something like _get_charge_type()
.
Right now we only need to modify the name while we keep the body of the function but I can see how this can be also helpful as we move into transient (time-domain) simulations.
aae214d
to
fe57931
Compare
19c31df
to
82e4266
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frequency: ArrayFloat1D = pd.Field( | ||
..., | ||
title="AC Analysis Frequencies", | ||
description="List of small-signal analysis frequencies.", | ||
units=HERTZ, | ||
) | ||
|
||
amplitude: pd.FiniteFloat = pd.Field( | ||
..., | ||
title="Small-Signal Amplitude", | ||
description="Small-signal AC amplitude.", | ||
units=VOLT, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah wondering if we should specify this via something like
class SinusodialSignalType
frequency: ArrayFloat1D = pd.Field(
...,
title="AC Analysis Frequencies",
description="List of small-signal analysis frequencies.",
units=HERTZ,
)
amplitude: pd.FiniteFloat = pd.Field(
...,
title="Small-Signal Amplitude",
description="Small-signal AC amplitude.",
units=VOLT,
)
and then:
class ACVoltageSource(Tidy3dBaseModel):
...
signal_parameters: SignalType
Just hacked this quickly but maybe also need to think about names
Apart from this rest of the PR looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel this would be pretty useful for the future too, also suspect maybe we already anyways have somthing like this for some time domain sources used elsewhere in the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes some sense @daquinteroflex, similarly to how we have our FDTD sources take a source_time
which just defines the time dependence of the source, while the other parameters are related to the spatial extent, specific details of the modes, etc. On the other hand, doesn't amplitude make more sense to be an ACVoltageSource
parameter than a SinusoidalSignal
(because it's nice if the signal itself is dimensionless). But then that just leaves frequency
in the SinusoidalSignal
, so it makes it a bit pointless?
I don't think there's anything from the existing API we can reuse directly. One other thing I'm thinking about is that while this is technically a source, it also more closely matches the FDTD monitor-s. You can define a list of frequencies, and you will get output data that has a dimension over that list. This is exactly like our MonitorData. One point here is that we've been already inconsistent about the frequency labeling: we have Monitor.freqs
, but the MonitorData has a corresponding f
dimension. Shall we at least call frequency
-> freqs
here to be in line with our FDTD monitors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So am thinking eventually we probably want to implement all these different ways of specifying time domain SPICE signal sources (either for voltage or current).
But feel maybe we need to be more specific whether we want to use an ACVoltageSource
or simply all the specific signal shapes we want to support directly. I think @damiano-flex made it this way because of the way voltage sources are specified in SPICE
VXXXXXXX N+ N- <<DC> DC/TRAN VALUE> <AC <ACMAG <ACPHASE>>>
+ <DISTOF1 <F1MAG <F1PHASE>>> <DISTOF2 <F2MAG <F2PHASE>>>
translates to
name positive_node negative_node
[this specifies a DC bias <<DC> DC/TRAN VALUE>]
[this specifies a custom AC source <AC <ACMAG <ACPHASE>>> + <DISTOF1 <F1MAG <F1PHASE>>> <DISTOF2 <F2MAG <F2PHASE>>>]

So in this sense feeling this is why a ACSignalType
should be defined as a standalone class. See 4.1 in https://ngspice.sourceforge.io/docs/ngspice-44-manual.pdf These are some examples of how they can be defined:
Based on these types of signal definitions, in my opinion I actually do think ampltude does make sense to live say in SinusosidalSignal
(not my favorite name yet just to be consistent), because say a PulseSignal
can be specifed between two voltages rather than just a single magnitude for example, and other AC sources require different parameters independent of amplitude too yet are still "AC" source compatible.
Sounds good, was not sure either what exactly was reusable. I do agree for consistency we can use freqs
.



There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to use freqs
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes, sounds good.
Those are definitely related to what we're doing here, but I was saying that I don't think our existing API can be used, because there's still some assumptions there that this is geared towards EM simulations. For example, the ContinuousWave class is basically a Sinusoidal, but, first of all, it's complex-valued, and second of all, it includes a linear ramp-up based on the fwidth parameter. For something as simple as a base amplitude and an offset amplitude where the source is something like V0 + V1*Sinusoidal, it still makes sense to me to follow this approach of defining a time-only class. I
That said, on second thought, if we do decide to separate time-only (unitless) definitions, this file could be a good place to put some new classes.
This seems a good approach in terms of API consistency, basically these abstract time classes would be an equivalent representation, and we will be able to translate to SPICE types if we need eventually too. Also worth getting @nikoladjor-flex input here. So creating those abstract time-shared classes seems good, and they can be used within ACVoltageSource and or eventually ACCurrentSource too in this approach. I do think it makes sense to define signal classes separate from the kind of physical quantity like Voltage that they modulate for reusability. Not sure what you think about that @damiano-flex ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daquinteroflex as we discussed in Slack, I don't have a strong opinion about what's the right thing to do. Here's what i think though.
Having a class SinusoidalSignal
is not something inherently wrong to me, but providing a vector of frequencies to this class is. Classes in time.py
are made to generate a single time-signal each, either by specifying some parameters (ContinousWave
) or by specifying the value of the signal in time domain. Providing a vector of frequencies here still seems a bit off to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, a vector would be weird in this case since it doesn't make sense physical sense unless it is a collection of signals. Feel this is a byproduct of wanting to integrate a sweep at the same time as a source potentially in the initial implementation, maybe instead of separating it into the analysis. Personally of the opinion parameter sweeps analysis to be separate from the source definition anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point @daquinteroflex. Maybe we could allow to have an array of ac sources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, the things I can add at the moment are more related to the application and I might be completely off-topic and wrong:
- small signal analysis is usually done to determine frequency behaviour when you do not know the impedance of the device and want to get the bandwidth. Two most obvious applications for PIC are p/n modulators and photodetectors. Having array of sources here might help if there is a step in the simulation with brings the system to a steady state and then you apply small signals separately.
- from the API point of view, this analysis makes sense on a single component, because you want three terminals the most. I tend to agree with @daquinteroflex about the abstract ACSource with added offset and frequency as list maybe. The point with the array of sources is the same as above. My main remark is that from the point of view of setting this analysis up, one needs to know which structures get what type of signal, and if we are already using BCs for that, it's fine, but we need to provide easy way to go from the circuit to the BC definition and from analysis result to impedance.
... ) | ||
""" | ||
|
||
name: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the name
?
If it's needed, it should be formatted similarly to e.g. this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had no purposes for it in mind, I just used the same structure of DCVoltageSource
(which has a field formatted like this one) . But in principle, having a name could be useful for future usage (e.g. a function that modify a source value through the name).
So, either I remove it in both, or I format it correctly here and in DCVoltageSource
well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's format both, thanks.
@damiano-flex @marc-flex I am getting confused with the |
Initially, I had a different Analysis type for AC analysis. It was virtually a clone. Marc suggested to keep old analysis types, and define AC at source level, and this idea seemed good to me as well. If you have an AC source, you'll get everything from the DC simulation + a graph for AC current. An option might be that |
I think that makes sense. Note that, for backwards compatibility, we'd keep
Refer to the HeatSimulation. |
7013f01
to
6c070c7
Compare
e19ba56
to
02f85c4
Compare
@damiano-flex @momchil-flex I'm now having second thoughts about the current naming of sources and analysis types. Right now we're saying that if we see an
|
These type of charge simulations are different from FDTD simulations in the sense that a given "scene" with sources etc. can have different types of analysis depending on what you're looking into like transient or DC operating point, and so the data generated is inherently different per analysis type. In any case:
would be then equivalent to this type of SS ac SPICE analysis directive that Damiano mentioned separately, but then the decision is whether the analysis type has the frequency sweep or the source. IMO it should be the analysis but use the source name to reference to it, and override any specified frequency in it.
|
02f85c4
to
1029ca8
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/spice/sources/ac.pyLines 50-58 50
51 @pd.validator("amplitude")
52 def validate_amplitude(cls, val):
53 if val == td_inf:
! 54 raise ValueError(f"Signal amplitude must be finite. Current amplitude={val}.")
55 return val
56
57 @pd.validator("frequency")
58 def validate_frequency(cls, val): Lines 56-64 56
57 @pd.validator("frequency")
58 def validate_frequency(cls, val):
59 if val is not None and val == td_inf:
! 60 raise ValueError(f"Signal frequency must be finite. Current frequency={val}.")
61 return val
62
63
64 class ACVoltageSource(Tidy3dBaseModel): tidy3d/components/tcad/simulation/heat_charge.pyLines 658-666 658 if isinstance(bc_spec.condition.source, GroundVoltageSource):
659 ground_count = ground_count + 1
660
661 if ground_count > 1:
! 662 raise SetupError(
663 f"Only one GroundVoltageSource can be used. Found {ground_count} "
664 "'GroundVoltageSource' instances."
665 ) Lines 1987-2003 1987
1988 return any(isinstance(source, HeatFromElectricSource) for source in self.sources)
1989
1990 def _get_charge_type(self):
! 1991 for bc in self.boundary_spec:
! 1992 if isinstance(bc.condition, VoltageBC):
! 1993 if isinstance(bc.condition.source, ACVoltageSource):
! 1994 return "ac"
! 1995 return "dc"
1996
1997 def _get_ssac_frequency_and_amplitude(self):
1998 if not isinstance(self.analysis_spec, IsothermalSSACAnalysis):
! 1999 raise SetupError(
2000 "Invalid analysis type for Small-Signal AC (SSAC). "
2001 f"SSAC requires a 'IsothermalSSACAnalysis' to set the DC operating point, "
2002 f"but received '{type(self.analysis_spec).__name__}' instead."
2003 ) Lines 2007-2015 2007 if isinstance(bc.condition, VoltageBC):
2008 if isinstance(bc.condition.source, ACVoltageSource):
2009 amplitude = bc.condition.source.signal.amplitude
2010 if amplitude is None:
! 2011 raise SetupError(
2012 "AC voltage source not found. "
2013 "A Small-Signal AC (SSAC) analysis requires one 'ACVoltageSource' "
2014 "to be defined in the 'boundary_spec', but none was found."
2015 ) Lines 2016-2024 2016
2017 if len(self.analysis_spec.ssac_freqs) > 0:
2018 return (self.analysis_spec.ssac_freqs, amplitude)
2019 else:
! 2020 raise SetupError(
2021 "Frequency list for SSAC analysis is missing. "
2022 "Please specify the frequencies to be analyzed, in the 'analysis_spec.ssac_freqs' attribute."
2023 ) |
Yeah I don't really know the answer but we must 100% think about it right now before things are released. It does seem to me that the current approach will be becoming more and more tedious/confusing as new regimes are added. Maybe try to sketch what other analysis we want to support and how what the API might look like - even if it's for one or two years from now. Maybe let's take the discussion off this repo, too. |
1029ca8
to
0be47a0
Compare
Jira-Ticket: FXC-1655
0be47a0
to
b14df29
Compare
SteadyChargeDCAnalysis with SSAC | ||
------------------------------ | ||
|
||
The :class:`SteadyChargeDCAnalysis` class now supports small signal analysis through the ``ssac_freqs`` parameter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'd remove "now". It seems new today but it won't in a few months?
Best Practices | ||
============== | ||
|
||
1. **Ground Reference**: Specify exactly one ground reference using :class:`GroundVoltageSource`. If no ground is specified, the smallest voltage source will be considered as the ground voltage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't entirely true, right? If no ground is provided, "ground" will be the BC without an array of voltages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right.
|
||
1. **Ground Reference**: Specify exactly one ground reference using :class:`GroundVoltageSource`. If no ground is specified, the smallest voltage source will be considered as the ground voltage. | ||
|
||
2. **Small Signal Amplitude**: Keep the AC amplitude small (typically 1-10 mV) to ensure linear operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason why you'd keep the amplitude small is so that the solution has some physical meaning. I think you could say something like this instead: Note that since this is a linear analysis tool, the simulated AC current and voltages will be proportional to the amplitude and may not have physical meaning for large enough amplitudes.
Common Use Cases | ||
================ | ||
|
||
- **MOSFET Analysis**: Study transconductance and output conductance vs. frequency. | ||
- **Diode Characterization**: Analyze junction capacitance and conductance. | ||
- **BJT Small-Signal**: Compute h-parameters and frequency response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only problem with saying this is that the users may expect the corresponding notebook examples, so I would refrain from saying this, at least for now.
what do you think @momchil-flex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that once this is merged, notebooks can be produced with these examples. But in principle, you could already do these things already.
I have a notebook with SSAC that can be released (after few modifications) after this comes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not necessarily opposed to mentioning use cases before we have an example, but these use cases (apart from the diode characterization) are not our target use cases and we're unlikely to work on an example showcasing our charge solver on a MOSFET for example. The relevant devices for us would be modulators and photodetectors, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Given the other discussion though, this file will go away.
This source represents a small-signal AC excitation defined by a list of bias voltages, | ||
and a small-signal amplitude (linear scale). The list of frequencies of interest must be supplied to the analysis class. | ||
The bias ``voltage`` refers to the DC operating point above the simulation ground. Currently, full circuit simulation though electrical ports is not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full circuit simulation though electrical ports
-> full circuit simulation through electrical ports
?
class ACVoltageSource(Tidy3dBaseModel): | ||
""" | ||
Small-signal AC voltage source. | ||
Notes | ||
----- | ||
This source represents a small-signal AC excitation defined by a list of bias voltages, | ||
and a small-signal amplitude (linear scale). The list of frequencies of interest must be supplied to the analysis class. | ||
The bias ``voltage`` refers to the DC operating point above the simulation ground. Currently, full circuit simulation though electrical ports is not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a little tricky. Right now this class is called ACVoltageSource
which in principle could be used for either linear or non-linear analysis but everything in this class is done assuming SSAC. So I guess we have two options:
- We change the name to something like
SSACVoltageSource
or - we keep the
ACVoltageSource
as pure AC source but we treat it as AC since at the moment it can only be defined in simulations wheressac_freq
has been defined in the steady analysis spec
The problem with 2 is that in the long term we may want to support AC sources with arbitrary shapes so we'd need to modify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could discuss API alternatives in the Lark document. Each approach has his own strength and weakness.
----- | ||
This establishes the ground reference for the simulation. All other voltage sources | ||
will be referenced to this ground potential. If no GroundVoltageSource is specified, | ||
the smallest voltage among all sources becomes the ground reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to above. The ground (or reference) is the one without voltage arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is actually true. I was just thinking about the case with one single BC for each contact.
analysis_spec = values.get("analysis_spec") | ||
if ( | ||
analysis_spec | ||
and hasattr(analysis_spec, "ssac_freqs") | ||
and len(analysis_spec.ssac_freqs) > 0 | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general we kind of avoid hasattr
though maybe @momchil-flex can comment.
As an alternative you could replace by a isinstance
if?
if isinstance(analysis_spec, SteadyChargeAnalysis) and len(analysis_spec.ssac_freqs) > 0:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, writing it in this more manifest way is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new version implements already the Marc's approach.
if isinstance(bc.condition, VoltageBC): | ||
if isinstance(bc.condition.source, ACVoltageSource): | ||
return ( | ||
bc.condition.source.freqs.tolist(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACVoltageSource
no longer has frequencies, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right.
@@ -0,0 +1,89 @@ | |||
Small Signal Analysis Guide |:zap:| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this file. However, it is not in the right place. Our lectures page just links to the video seminar series that we have released. So far, we use a combination of the API reference and the main notebook introducing a given feature to provide the information that you have compiled here (e.g. the HeatChargeSimulation reference and the original charge solver notebook.
Again, I like the idea of this file, and it could be nice if some day we have such "feature white papers". But given that it's a loner right now, probably this info should be moved to the components and the upcoming example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. OK, I'll create an example directly into HeatChargeSimulation
, and will save the rest of the information for the upcoming notebook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments. But I think we should consider @daquinteroflex 's proposal in the Lark document, if not implementing fully then at least some aspect of it. I'll put down some more thoughts a bit later.
amplitude: pd.FiniteFloat = pd.Field( | ||
default=1.0, | ||
title="Small-Signal Amplitude", | ||
description="Small-signal AC amplitude. If not specified, it is assumed to be unitary.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you mean unity
?
Should we default to a small value like 0.001 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unitary does not look wrong to me, as it is an adjective that means "of or relating to a unit". But to be fair I just searched it now 😃. I just wrote it assuming it to be correct to me. But can switch to "unity" if you don't like it.
Yeah, we could actually default to 1mV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest you could just drop the sentence? The default is the parsed docstring so no need to say anything.
My problem with unitary
is that to me it sounds like a unitary matrix, or related to that a unitary time evolution, which has a completely unrelated meaning (but e.g. in some contexts unitary kind of means "lossless", so just want to avoid confusion).
"either the conduction or valence energy bands.", | ||
) | ||
|
||
ssac_freqs: Optional[ArrayFloat1D] = pd.Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So your suggestion is for this spec to eventually become SteadyChargeAnalysis
?
If so, you could introduce it already:
class SteadyChargeDCAnalysis(Tidy3dBaseModel):
# same as before, maybe a new deprecation message on init
class SteadyChargeAnalysis(SteadyChargeDCAnalysis):
ssac_freqs: Optional[ArrayFloat1D] = pd.Field(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the ongoing discussion in the lark document, I'll postpone this to the moment a final decision on SSAC analysis class is taken. If SSAC will eventually go to a new class, then we could keep DC actually.
name: Optional[str] = pd.Field( | ||
None, | ||
title="Name", | ||
description="Unique name for the DC current source", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we validate that the names are unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since names are not used anywhere basically. They're just introduced for future (probably near) future. This said, validating the uniqueness regardless seems a good idea to me.
if isinstance(bc.condition, VoltageBC): | ||
if isinstance(bc.condition.source, ACVoltageSource): | ||
if ssac_present: | ||
raise SetupError("Only a single AC source can be supplied") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing period at the end of the message
raise SetupError( | ||
"Frequency list for SSAC analysis is missing. " | ||
"Please specify the frequencies to be analyzed, in the 'analysis_spec.ssac_freqs' attribute." | ||
) |
There was a problem hiding this comment.
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 all the error checking here. Shouldn't these things have been caught already? I guess it doesn't hurt to have extra checks, but if any of those are hit then something's wrong with the initial validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good point linked to the API discussion (I hate it sounds like a Gemini response).
Currently, the error error cannot caught earlier because:
- The AC source is provided as a BC.
ssac_freqs
is provided to the analysis type (and not to the AC source as in the earlier implementation).
Therefore, the only place where you can have this check is when you assemble the HeatChargeSimulation
. Because if there's no AC source, you don't actually need ssac_freqs
.
ee88093
to
f7f1d1f
Compare
30a8430
to
c578465
Compare
Add SSAC Voltage source class.
Greptile Summary
Updated On: 2025-09-10 09:22:23 UTC
This PR adds support for Small Signal AC (SSAC) voltage sources to the Tidy3D simulation framework. The implementation introduces a new
SSACVoltageSource
class that enables AC analysis in heat-charge simulations by providing DC bias voltages, AC frequencies, and small-signal amplitudes.The core change is the addition of the
SSACVoltageSource
class in a new module (tidy3d/components/spice/sources/ac.py
), which follows the established patterns from the existingDCVoltageSource
class. The class includes three main parameters:voltage
(array of DC bias voltages),frequency
(array of AC analysis frequencies), andamplitude
(small-signal AC magnitude). All fields include proper validation to ensure finite values and positive frequencies.The implementation integrates seamlessly with the existing SPICE sources framework by updating the
VoltageSourceType
union to include the new AC source alongside the existing DC source. A validation constraint has been added to theHeatChargeSimulation
class to enforce that only a single AC source can be present in a simulation's boundary conditions, which prevents ambiguous analysis scenarios.The new functionality is exposed through the main package interface by adding the class to the imports and
__all__
list intidy3d/__init__.py
. This follows the established pattern where SPICE-like analysis components are made available as part of the public API.Important Files Changed
Changed Files
tidy3d/components/spice/sources/ac.py
tidy3d/components/tcad/simulation/heat_charge.py
tests/test_components/test_heat_charge.py
tidy3d/components/spice/sources/types.py
tidy3d/__init__.py
Confidence score: 4/5
tidy3d/components/tcad/simulation/heat_charge.py
to ensure the validation logic is completeSequence Diagram