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.

Sign up for GitHub

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

Jump to bottom

Integrate density in state object #2386

Merged
merged 16 commits into from
Aug 15, 2023
Merged

Integrate density in state object #2386

merged 16 commits into from
Aug 15, 2023

Conversation

wkerzendorf
Copy link
Member

another stab at the restructure

wkerzendorf and others added 10 commits July 20, 2023 14:44
@wkerzendorf @AlexanderGrunewald
@afloers @Rodot- @andrewfullard
Add radiationfield property
6cd2ba0 
Co-authored-by: Alexander Grunewald <AlexanderGrunewald@users.noreply.github.com>
Co-authored-by: Andreas Flörs <afloers@users.noreply.github.com>
Co-authored-by: Jack O'Brien <Rodot-@users.noreply.github.com>
Co-authored-by: Andrew Fullard <andrewgfullard@gmail.com>
@wkerzendorf
insert radiation_field
a76c5c4
@wkerzendorf
Merge remote-tracking branch 'upstream/master' into integrate_radfield2
59522b4
@wkerzendorf
cleanup radiation field and introduce changes to model
460f62a
@wkerzendorf
integrate radiation field
e122086
@wkerzendorf
further cleanup
1ba6f32
@wkerzendorf
change to packet_source
7416407
@wkerzendorf
blackify
7003941
@wkerzendorf
remove radiation field
5623d24
@wkerzendorf
restructure to pure density in radial1d model.
09e059b 
still some tests failing - maybe outdated.
@andrewfullard
Copy link
Contributor

Seems generally sensible. Would it be possible to fix at least the density tests? I think it should be possible to solve the abundance widget issue as well.

@codecov
Copy link

codecov bot commented Aug 12, 2023 •
edited
Loading

Codecov Report

Merging #2386 (34dbcc1) into master (44b2f47) will decrease coverage by 0.08%.
Report is 3 commits behind head on master.
The diff coverage is 93.06%.

❗ Current head 34dbcc1 differs from pull request most recent head 8626e02. Consider uploading reports for the commit 8626e02 to get more accurate results

@@            Coverage Diff             @@
##           master    #2386      +/-   ##
==========================================
- Coverage   68.50%   68.43%   -0.08%     
==========================================
  Files         144      145       +1     
  Lines       13264    13220      -44     
==========================================
- Hits         9087     9047      -40     
+ Misses       4177     4173       -4
Files Changed Coverage Δ
...s/montecarlo/montecarlo_numba/tests/test_packet.py 97.50% <ø> (-0.20%) ⬇️
tardis/montecarlo/tests/test_montecarlo.py 29.38% <ø> (-0.77%) ⬇️
tardis/visualization/widgets/custom_abundance.py 58.59% <60.00%> (-0.94%) ⬇️
tardis/io/model_reader.py 65.71% <66.66%> (-0.49%) ⬇️
tardis/io/model/density.py 91.48% <91.48%> (ø)
tardis/io/tests/test_model_reader.py 98.90% <100.00%> (-0.04%) ⬇️
tardis/model/base.py 90.81% <100.00%> (ø)
tardis/model/tests/test_density.py 100.00% <100.00%> (ø)
tardis/transport/tests/test_doppler_factor.py 100.00% <100.00%> (ø)
...sualization/widgets/tests/test_custom_abundance.py 87.64% <100.00%> (-12.36%) ⬇️

... and 1 file with indirect coverage changes

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

andrewfullard
andrewfullard reviewed Aug 14, 2023
View reviewed changes
tardis/io/model/density.py Show resolved Hide resolved
tardis/model/tests/test_density.py
expected = pd.read_hdf(hdf_file_path, path)
assert_almost_equal(actual, expected.values)


def test_hdf_time_0(hdf_file_path, simulation_verysimple):
actual = simulation_verysimple.model.homologous_density.time_0
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that time_0 and time_explosion are equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

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

in this case they are because we shift everything to time_explosion. the model object only has things at the same time.

andrewfullard
andrewfullard previously approved these changes Aug 14, 2023
View reviewed changes
@tardis-bot
Copy link
Contributor

tardis-bot commented Aug 14, 2023 •
edited
Loading

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

andrewfullard
andrewfullard approved these changes Aug 15, 2023
View reviewed changes
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.

If all of the refdata tests are going to pass, I'm happy.

@wkerzendorf wkerzendorf merged commit a5d25bf into tardis-sn:master Aug 15, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewfullard andrewfullard andrewfullard approved these changes

Assignees
No one assigned
Labels
build-docs model restructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wkerzendorf @andrewfullard @tardis-bot