-
Notifications
You must be signed in to change notification settings - Fork 44
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
Apodization for frequency monitors #574
Conversation
tidy3d/components/apodization.py
Outdated
|
||
@pd.validator("end", always=True, allow_reuse=True) | ||
def end_greater_than_start(cls, val, values): | ||
"""Ensure sure end is greater than or equal to start.""" |
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.
Ensure sure
tidy3d/components/apodization.py
Outdated
|
||
|
||
class ApodizationSpec(Tidy3dBaseModel): | ||
"""Stores specifications for the apodizaton of frequency 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.
frequency-domain
tidy3d/components/apodization.py
Outdated
) | ||
|
||
width: pd.PositiveFloat = 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.
How about setting the default to None
and just skipping the apodization on the backend if width is None
?
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 it, however I am worried that a user might not realize/just forget they have to set width
, so they would use ApodizationSpec(start=1, end=2)
and not get any apodization in their results. I guess we could set all start
, end
, and width
to None
by default, and a validator that checks if either start
or end
is not None
, then width
must not be None
as 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.
This makes sense to me yeah.
ApodizationSpec(width=1), | ||
title="Apodization Specification", | ||
description="Sets parameters of (optional) apodization.", | ||
) |
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.
"Sets parameters of (optional) apodization. Apodization applies a windowing function to the Fourier transform of the time-domain fields into frequency-domain ones, and can be used to truncate the beginning and/or end of the time signal, for example to eliminate the source pulse when studying the eigenmodes of a system. Note: apodization affects the normalization of the frequency-domain fields.",
or something along those lines
76f7512
to
3a7747e
Compare
tidy3d/components/apodization.py
Outdated
raise SetupError("End apodization begins before start apodization ends.") | ||
return val | ||
|
||
@pd.validator("width", always=True, allow_reuse=True) | ||
def width_provided(cls, val, values): | ||
"""Ensure end is greater than or equal to start.""" |
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.
Wrong docstring
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 bad, fixed!
3a7747e
to
b5ad1f0
Compare
b5ad1f0
to
e32b8c4
Compare
Specification of apodization for frequency monitors
note that I set
width
to be a non-optional parameters, because generally there doesn't seem to be a common default value