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

Swap to new model #112

Merged
merged 6 commits into from
Aug 9, 2023
Merged

Conversation

jvshields
Copy link
Contributor

@jvshields jvshields commented Aug 7, 2023

📝 Description

Type: 🚀 feature | ☣️ breaking change | 🎢 infrastructure

This PR attaches swaps the old model created by read_marcs_to_fv() out and hooks up the new model reader to the run_stardis() function. This effectively implements much of the restructure work with the new infrastructure, and so it also includes changing the code in many places to read information from the new model structure.

Future PRs need to change the Configuration schema to allow for general model reading, as the current code only allows for unzipped MARCS models, so the configuration should allow to specify whether the input model is gzipped or not.

Additionally, I have left many comments around the code pointing out parts that need cleaning or changing, because they are misleading or redundant. These are not-functional changes, and should be changed in a future cleaning PR.

I am leaving the read_marcs_to_fv() function for now, because it is currently integrated into the benchmarking pipeline, but a future PR should also swap that over to the new structure as well.

🚦 Testing

How did you test these changes?
I have tested the output of the quickstart notebook with the same model as before. I get a different answer for the spectrum, shown here

Comparison Spectrum

There is a very small difference here that is not entirely unexpected. The previous model reader made physical changes to the system by naively interpolating the temperatures and densities of the system and then defining cells with those values, and solving an opacity for those cells. This new method solves the plasma using the values provided by the model, and then linearly interpolates the opacities measured at each of the points for light to travel from one depth point to the next. I generally believe that the new model is working correctly, but as far as I can tell it's impossible to directly reproduce the previous model because that model solves a completely different plasma and then solves the radiative transfer equation differently.

The answers are close enough that I think the new model is working correctly.

  • Testing pipeline
  • Other method (describe)

☑️ 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 changed the title swap to new model. spectrum appears incorrect swap to new model Aug 7, 2023
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #112 (0282128) into main (a26fe04) will decrease coverage by 0.16%.
Report is 2 commits behind head on main.
The diff coverage is 2.50%.

❗ Current head 0282128 differs from pull request most recent head cce9b27. Consider uploading reports for the commit cce9b27 to get more accurate results

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   34.04%   33.89%   -0.16%     
==========================================
  Files          17       17              
  Lines         658      658              
==========================================
- Hits          224      223       -1     
- Misses        434      435       +1     
Files Changed Coverage Δ
stardis/base.py 33.33% <0.00%> (-4.17%) ⬇️
stardis/io/model/marcs.py 67.70% <0.00%> (-0.72%) ⬇️
stardis/model/geometry/radial1d.py 66.66% <0.00%> (ø)
stardis/opacities/base.py 13.71% <0.00%> (+0.23%) ⬆️
stardis/opacities/broadening.py 31.08% <0.00%> (ø)
stardis/plasma/base.py 44.61% <ø> (+0.67%) ⬆️
stardis/transport/base.py 25.00% <0.00%> (-0.50%) ⬇️
stardis/model/base.py 11.36% <50.00%> (ø)

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

@jvshields jvshields changed the title swap to new model Swap to new model Aug 9, 2023
@jvshields jvshields marked this pull request as ready for review August 9, 2023 18:17
sonachitchyan
sonachitchyan previously approved these changes Aug 9, 2023
Copy link
Member

@sonachitchyan sonachitchyan left a comment

Choose a reason for hiding this comment

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

Looks good! The only thing is maybe we should move the comments to an issue and make a separate PR for solving them? This way it feels disconnected from the purpose of this PR

sonachitchyan
sonachitchyan previously approved these changes Aug 9, 2023
andrewfullard
andrewfullard previously approved these changes Aug 9, 2023
@jvshields jvshields dismissed stale reviews from andrewfullard and sonachitchyan via e72f455 August 9, 2023 20:19
@andrewfullard andrewfullard merged commit 437e371 into tardis-sn:main Aug 9, 2023
smokestacklightnin pushed a commit to smokestacklightnin/stardis that referenced this pull request Sep 20, 2023
* swap to new model. spectrum appears incorrect

* trying to fix spectrum incorrect broadening

* comment out read_marcs_to_fv and cleanup commented out code

* Uncomment read_marcs_to_fv(), add comments, debug spectrum

* rename looping index for clarity, remove non-functional parentheses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants