-
Notifications
You must be signed in to change notification settings - Fork 70
FXC-4546: add baseband source time classes for transient simulations #3250
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
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/source/time.pyLines 106-119 106 freqs = np.fft.rfftfreq(len(values), d=dt)
107
108 peak = np.max(spectrum)
109 if peak == 0:
! 110 return (0.0, 1.0)
111
112 cutoff = np.exp(-(num_fwidth**2) / 2)
113 above_cutoff = spectrum >= cutoff * peak
114 if not np.any(above_cutoff):
! 115 return (0.0, freqs[-1])
116
117 indices = np.where(above_cutoff)[0]
118 fmin = float(freqs[indices[0]])
119 fmax = float(freqs[indices[-1]]) |
711db32 to
965c864
Compare
965c864 to
8e17e32
Compare
a75d1be to
222b098
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
This one is basically ready to go. I opted for creating new classes to handles these signals with DC components, instead of modifying the existing ones to support |
Add BasebandStep, BasebandGaussianPulse, BasebandRectangularPulse, and BasebandCustomSourceTime for unmodulated transient RF excitations (TDR, step functions, arbitrary time profiles). These inherit from SourceTime directly, require no freq0/fwidth, and compute frequency_range from the signal's actual frequency content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
222b098 to
79ef5bd
Compare
George-Guryev-flxcmp
left a comment
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, Damian — this looks solid! I have a few minor comments and suggestions.
| title="Rise Time", | ||
| description="Characteristic rise time in seconds. " | ||
| "The 10%-to-90% rise time is approximately 2.56 times this value.", | ||
| ) |
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 believe the variable rise_time is misleading in this context: the rise time is typically a time it takes to go from 10% to 90%, while the variable rise_time is used here as sigma in step function below. The actual rise time is approximately 2.56 * sigma. I wonder one can rename rise_time to standard_deviation or something similar? Or alternatively use the actual rise time for this variable.
| The ``phase`` parameter is locked to 0 since baseband signals have no carrier. | ||
| """ | ||
|
|
||
| phase: Literal[0] = Field( | ||
| 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 guess the default nominal value makes sense. However, I wonder if one could have a scenario where a simulation has multiple ports (say two) whose sources/waveforms should have a constant phase offset to each other? One can surely do phase scaling in post-processing, however, I wonder if you think this might be a useful behavior here (i.e. default to 0 but allow for any constant phase offset for a given waveform?)
| """Equivalent bandwidth in Hz.""" | ||
| return 1.0 / (2 * np.pi * self.rise_time) |
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 believe this uses the RMS bandwidth; however, multiple alternative definitions exist. Making this explicit in the docstring would remove any ambiguity.
| """Equivalent bandwidth in Hz.""" | ||
| return 1.0 / (2 * np.pi * self.rise_time) | ||
|
|
||
| def amp_time(self, time: float) -> complex: |
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.
All amp_time() methods give a type hint float for time. However, in most cases time is treated as a numpy array. I wonder you could use time: Union[float, ArrayLike] or something similar?

Note
Medium Risk
Introduces new
SourceTimevariants and updates schema/type unions used broadly in simulation validation, which could affect source serialization/validation and downstream integrations relying onSourceTimeType.Overview
Adds new baseband (no-carrier)
SourceTimeoptions for transient RF workflows:BasebandStep,BasebandGaussianPulse,BasebandRectangularPulse, andBasebandCustomSourceTime(includingfrom_values,end_time, and FFT-basedfrequency_range).Plumbs these new source times through the public API (
tidy3d.__init__,tidy3d.rf), schema discriminators (Simulation.json,ModeSimulation.json,TerminalComponentModeler.json), and type wiring by movingSourceTimeTypeinto a newcomponents/types/time.pymodule; also tightens validation by requiring positivedtforCustomSourceTime.from_valuesand tweaking the low-frequency error message for sources.Extensive new tests validate baseband behavior (phase locked to 0, amplitude shapes,
end_time/frequency_range, plotting, and fullSimulationvalidation) plusCustomSourceTimedtedge cases.Written by Cursor Bugbot for commit 79ef5bd. This will update automatically on new commits. Configure here.