-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: add validation for units #2335
base: develop
Are you sure you want to change the base?
Conversation
We encountered users specifying the wrong units in their simulation setup, so the motivation here is to introduce range-based validators and link to the relevant docs page to make sure users can quickly identify and correct unit issues. While the validators impose strict bounds, the idea is not to necessarily allow those values but simply catch setups that are clearly off by orders of magnitude. We had a stab at selecting bounds that would cover that but if you could double check @momchil-flex and @tomflexcompute? |
I think it makes sense, the main concern is RF freqs, it seems like the warning will currently be raised for a wavelength larger than 1 meter, which is probably fine for all applications we're currently targeting? But may be close. But probably for a center frequency it's fine. I remember we added a similar validator at some point, maybe only for the mode solver... But I can't find it now. @weiliangjin2021 do you remember? |
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 @rahul-flex this looks good! Left some minor comments. Make sure to also add tests for these validators and a changelog entry (under the "Changed" section probably).
tidy3d/components/grid/grid_spec.py
Outdated
""" | ||
Ensure 'dl' is within a physically sensible range to catch common | ||
unit mistakes. | ||
""" |
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 fits on a single line, can remove everything after sensible range
.
tidy3d/components/grid/grid_spec.py
Outdated
Ensure 'dl' is within a physically sensible range to catch common | ||
unit mistakes. | ||
""" | ||
if not (1e-6 <= val <= 5e4): |
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.
Probably 1e4
is fine here? Not that it matters much.
tidy3d/components/grid/grid_spec.py
Outdated
""" | ||
if not (1e-6 <= val <= 5e4): | ||
raise SetupError( | ||
f"Uniform grid spacing 'dl' is {val} µm." |
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.
Space missing here: µm."
-> µm. "
tidy3d/components/source/time.py
Outdated
min_freq = C_0 / (1e6) | ||
max_freq = C_0 / (1e-6) |
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.
Can remove the parantheses here
tidy3d/components/source/time.py
Outdated
""" | ||
Ensure 'freq0' is within a physically sensible range to catch | ||
common unit mistakes. | ||
""" |
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.
Same comment as above, I'd put this on a single line.
Hmm I guess that also means we might want |
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 is nice but I think 1m wavelength is actually a bit too restrictive for some use cases like radar and long range communication, even though these use cases are not common within our current user base.
@tomflexcompute I guess that would put us in the ~100m wavelength range? |
yeah potentially. It's probably fine for now but in the future if we have more users from RF and microwave it might be a problem. |
Thanks for the comments @yaugenst-flex @momchil-flex @tomflexcompute @weiliangjin2021. Should we make upper bound of dl > 1cm (say 10 cm?) and lower bound of frequency: 1e5 Hz? |
I wonder if we really need a validator for grid size. If the grid size is too large, you'll get just one grid in that dimension, and it should be easy to spot the issue? And if the grid size is too small, I think there is already a validator that checks the total number of grid points below a threshold value. Now if we go for a validator for grid size, with lower bound frequency 1e5 Hz, corresponding to wavelength 300m, the upper bound of grid size will be 30 m. |
@weiliangjin2021 Thanks for the bound values. |
See tidy3d/tidy3d/components/simulation.py Lines 3701 to 3709 in 14f326d
|
Ohh, I see! Thanks! |
@pydantic.validator("freq0", always=True) | ||
def _validate_freq0(cls, val): | ||
""" | ||
Ensure 'freq0' is within a physically sensible range. | ||
""" | ||
min_freq = C_0 / 3e9 | ||
max_freq = C_0 / 1e-6 | ||
if not (min_freq <= val <= max_freq): | ||
raise SetupError( | ||
f"Pulse central frequency 'freq0' is {val:.3e} Hz, which corresponds " | ||
f"to a free-space wavelength of {C_0 / val:.3e} µm. " | ||
"Check your units! For more info on Tidy3D units, see: " | ||
"https://docs.flexcompute.com/projects/tidy3d/en/latest/faq/docs/faq/What-are-the-units-used-in-the-simulation.html" | ||
) | ||
return 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.
As @weiliangjin2021 pointed out, there already is a frequency validator which we could reuse. Would it make sense to also add an upper frequency bound to that validator?
I think the size validator is not quite the same though, or at least if there is an opportunity to catch a bad |
Added setup errors for grid spacing
dl
and frequency (attached).