Add next term van noort intensity solver

jvshields commented 1 year ago

:pencil: Description

Type: | :rocket: feature | :vertical_traffic_light: 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.

:pushpin: Resources

From https://iopscience.iop.org/article/10.1086/338949/pdf equation 14. Adding the w2 d2S/dt2 term.

:vertical_traffic_light: 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.

normalized_residuals

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

:ballot_box_with_check: Checklist

[x] 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.

codecov[bot] commented 1 year ago

Codecov Report

Merging #120 (84f2d62) into main (437e371) will increase coverage by 39.14%. Report is 2 commits behind head on main. The diff coverage is 0.00%.

:exclamation: Current head 84f2d62 differs from pull request most recent head 0235311. Consider uploading reports for the commit 0235311 to get more accurate results

@@             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

Files Changed	Coverage Δ
stardis/transport/base.py	`38.59% <0.00%> (+13.59%)`	:arrow_up:

... and 13 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

tardis-bot commented 1 year ago

*beep* *bop* Hi human, I ran benchmarks as you asked comparing main (7bec1bb6e273cc643eeb93cf9b84474340130db6) and the latest commit (0235311b838665033d5d0d562e6ba5dc177ee828). Here are the logs produced by ASV. Results can also be downloaded as artifacts here. Significantly changed benchmarks:

```diff ```

All benchmarks:

```diff All benchmarks: before after ratio [7bec1bb6] [0235311b] 12.9±0.03s 12.9±0.05s 1.01 run_stardis.BenchmarkRunStardis.time_run_stardis ```

tardis-sn / stardis