-
-
Notifications
You must be signed in to change notification settings - Fork 424
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
Moves non-physical input check outsode I/O module #2923
Changes from 4 commits
9716029
1a1d8b9
d6609b3
aa7a3ed
cab6bd6
3fd4ab7
7a77bff
30560cc
d78865c
0fdc3fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,6 +215,8 @@ Stuart Sim <[email protected]> ssim <[email protected]> | |
Stuart Sim <[email protected]> Stuart Sim <[email protected]> | ||
Stuart Sim <[email protected]> Stuart Sim <[email protected]> | ||
|
||
Swayam Shah <[email protected]> Sonu0305 <[email protected]> | ||
|
||
TARDIS Bot <[email protected]> | ||
TARDIS Bot <[email protected]> tardis-bot <[email protected]> | ||
TARDIS Bot <[email protected]> TARDIS Bot <[email protected]> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should validation be in a directory called "physics"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understood the title of #2681, 'Move ... to a physics module,' as meaning to move it to a physics directory (module). Sorry if I misunderstood. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. The term was meant generically, because most of TARDIS consists of modules that calculate physics. Thus these checks should be placed in the most relevant pre-existing module. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay understood, I'll make the necessary changes. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import logging | ||
import numpy as np | ||
from astropy import units as u | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
def validate_radiative_temperature(t_radiative): | ||
""" | ||
Validates the radiative temperature to ensure it is physically reasonable. | ||
|
||
Parameters | ||
---------- | ||
t_radiative : Quantity | ||
The radiative temperature array. | ||
|
||
Raises | ||
------ | ||
ValueError | ||
If any radiative temperature is below 1000 K. | ||
""" | ||
if np.any(t_radiative < 1000 * u.K): | ||
min_t_rad = t_radiative[np.argmin(t_radiative)] | ||
min_shell = np.argmin(t_radiative) | ||
logging.critical( | ||
"Radiative temperature is too low in some of the shells, temperatures below 1000K " | ||
f"(e.g., T_rad = {min_t_rad} in shell {min_shell} in your model) " | ||
"are not accurately handled by TARDIS." | ||
) | ||
raise ValueError( | ||
f"Radiative temperature below 1000 K detected: T_rad = {min_t_rad} in shell {min_shell}." | ||
) |
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.
Why is this change included?
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 avoid codespell job failure like it is mentioned in #2908 but it is not merged yet, hence included in 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.
A PR should ideally have only one purpose, and this will lead to merge conflicts that must be resolved once #2908 is merged
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.
Sorry, I got your point, will make sure this does not repeat.