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

Add next term van noort intensity solver #120

Conversation

jvshields
Copy link
Contributor

@jvshields jvshields commented Aug 11, 2023

📝 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.

normalized_residuals

  • 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.

@jvshields jvshields added the benchmarks Trigger benchmarks to run on this pr label Aug 11, 2023
@codecov
Copy link

codecov bot commented Aug 11, 2023

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%.

❗ 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%) ⬆️

... and 13 files with indirect coverage changes

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

@jvshields jvshields marked this pull request as ready for review August 11, 2023 14:48
@jvshields jvshields added benchmarks Trigger benchmarks to run on this pr and removed benchmarks Trigger benchmarks to run on this pr labels Aug 11, 2023
@tardis-bot
Copy link
Contributor

tardis-bot commented Aug 11, 2023

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

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

@sonachitchyan sonachitchyan merged commit d6bde59 into tardis-sn:main Aug 11, 2023
6 checks passed
smokestacklightnin pushed a commit to smokestacklightnin/stardis that referenced this pull request Sep 20, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Trigger benchmarks to run on this pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants