-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add next term van noort intensity solver #120
Add next term van noort intensity solver #120
Conversation
Codecov Report
@@ Coverage Diff @@
## main #120 +/- ##
===========================================
+ Coverage 33.89% 73.03% +39.14%
===========================================
Files 17 21 +4
Lines 658 686 +28
===========================================
+ Hits 223 501 +278
+ Misses 435 185 -250
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
*beep* *bop* All benchmarks: All benchmarks:
before after ratio
[7bec1bb6] [0235311b]
12.9±0.03s 12.9±0.05s 1.01 run_stardis.BenchmarkRunStardis.time_run_stardis
|
* attempt at extra term. Seems too large. * add second term to van noort equation. not clean yet, want to see benchmarks * cleanup * fix calc_weights
📝 Description
Type: | 🚀
feature
| 🚦testing
|This PR adds the next term of the van noort 2001 equation we solve to do radiative transfer. It seems like it changes our answer at the 0.1% level, which is small enough that it's not a particularly significant change, but the radiative transfer equation is also such a small part of the runtime of the code that I think it's worth doing by default. If people think it's necessary, we can make this an option that can be turned on or off, but I think we should just adopt it into the code always because the performance increase is negligible. I've run the benchmarks a couple times and have not seen a statistically significant change in runtime.
As a note, if we can get PR 85 merged first, it'd be nice to see the timing difference in the raytrace function itself.
📌 Resources
From https://iopscience.iop.org/article/10.1086/338949/pdf equation 14. Adding the w2 d2S/dt2 term.
🚦 Testing
Normalized residuals about H-alpha line can be seen here. There's a systematic offset at the 0.1% level.
I'm adding the benchmarks label so we can see if this is a significant slowdown.
☑️ Checklist
build_docs
label