Skip to content
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

prevent float overflow when calculating He I population #2522

Merged
merged 8 commits into from
Mar 8, 2024

Conversation

DeerWhale
Copy link
Contributor

@DeerWhale DeerWhale commented Feb 20, 2024

📝 Description

When the T_radiative in the shell is below 404 K, the highlighted part in the screenshot below will return Inf due to float overflow, which cause the nan values in the calculation of electron density.

This PR initially aims to prevent the overflow by capping the value to be the max float value allowed in the program. However a better way to do this maybe raise an error when the input model T_radiative is below certain threshold (1000K is chosen in this PR), so the user can double check the input model.

Fun fact, the low T_rad also cause kernel crash when using the macroatom setting.

image

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 68.73%. Comparing base (64df98b) to head (5706282).
Report is 1 commits behind head on master.

Files Patch % Lines
tardis/model/parse_input.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2522      +/-   ##
==========================================
- Coverage   68.74%   68.73%   -0.01%     
==========================================
  Files         165      165              
  Lines       13999    14001       +2     
==========================================
+ Hits         9623     9624       +1     
- Misses       4376     4377       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewfullard
Copy link
Contributor

Good find. My issue with this solution is that nothing in a TARDIS simulation should ever be that cold!

@DeerWhale
Copy link
Contributor Author

Good find. My issue with this solution is that nothing in a TARDIS simulation should ever be that cold!

The issue only occurs in the most outer layer in some of the cases (in some of the models there are shells >40k km/s with low T_rad ).

Alternative is printing out a warning state why the overflow is encountered there, so the user can modify their models accordingly with that Info?

@andrewfullard
Copy link
Contributor

I would suggest a combination of both. A warning that the n_electron calculation has overflowed, telling the user that their temperatures are too low (with a printout of the problem temperature value and possibly the shells that are affected)

andrewfullard
andrewfullard previously approved these changes Feb 21, 2024
@tardis-bot
Copy link
Contributor

tardis-bot commented Feb 22, 2024

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

jvshields
jvshields previously approved these changes Feb 22, 2024
andrewfullard
andrewfullard previously approved these changes Feb 22, 2024
andrewfullard
andrewfullard previously approved these changes Feb 22, 2024
Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A warning is perhaps optimistic but we shall see how it goes

@andrewfullard andrewfullard merged commit 39c128a into tardis-sn:master Mar 8, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants