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

Restructure/transport solver #2476

Merged
merged 165 commits into from
Jan 17, 2024

Conversation

wkerzendorf
Copy link
Member

@wkerzendorf wkerzendorf commented Nov 23, 2023

This is merged next.

Copy link

codecov bot commented Dec 24, 2023

Codecov Report

Attention: 146 lines in your changes are missing coverage. Please review.

Comparison is base (e1a8f24) 68.62% compared to head (1e3cb54) 68.46%.
Report is 2 commits behind head on master.

Files Patch % Lines
.../montecarlo/montecarlo_numba/packet_collections.py 15.58% 65 Missing ⚠️
tardis/montecarlo/montecarlo_state.py 77.46% 32 Missing ⚠️
tardis/montecarlo/montecarlo_numba/estimators.py 46.80% 25 Missing ⚠️
tardis/montecarlo/montecarlo_numba/base.py 26.66% 11 Missing ⚠️
tardis/simulation/tests/test_simulation.py 63.63% 4 Missing ⚠️
tardis/io/model_reader.py 0.00% 2 Missing ⚠️
tardis/model/geometry/radial1d.py 0.00% 1 Missing ⚠️
tardis/model/parse_input.py 90.00% 1 Missing ⚠️
tardis/montecarlo/base.py 92.30% 1 Missing ⚠️
...dis/montecarlo/montecarlo_numba/formal_integral.py 92.30% 1 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2476      +/-   ##
==========================================
- Coverage   68.62%   68.46%   -0.16%     
==========================================
  Files         159      159              
  Lines       13897    13719     -178     
==========================================
- Hits         9537     9393     -144     
+ Misses       4360     4326      -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

self.use_gpu = use_gpu

self.virt_logging = virtual_packet_logging
self.virt_logging = enable_virtual_packet_logging
self.virt_packet_last_interaction_type = np.ones(2) * -1
Copy link
Member Author

Choose a reason for hiding this comment

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

comment why length 2

Copy link
Member Author

Choose a reason for hiding this comment

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

done

self.transport_state = MonteCarloTransportState(
self.packet_collection,
estimators,
simulation_state.volume.cgs.copy(),
Copy link
Member Author

Choose a reason for hiding this comment

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

geometry state and volume duplicate - fix I think in the next one.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is fixed in the next one

andrewfullard
andrewfullard previously approved these changes Jan 5, 2024
@tardis-bot
Copy link
Contributor

tardis-bot commented Jan 8, 2024

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@@ -4,8 +4,11 @@
import pandas as pd

import tardis

from pathlib import Path
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is being double imported - check line 1

Copy link
Contributor

Choose a reason for hiding this comment

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

On line 675 - I think part of this restructure is trying to make sure we use more informative names. transport_from_hdf mostly operates on a dictionary called d, but it might be worth changing this to a more descriptive name. Not sure if this is outside the scope of this restructure though.

@@ -5,6 +5,7 @@
import pandas as pd
from astropy import units as u

from tardis import constants as const
Copy link
Contributor

Choose a reason for hiding this comment

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

Double import here. Also imported immediately below in line 9

@@ -31,29 +28,19 @@


# TODO: refactor this into more parts
class MontecarloTransport(HDFWriterMixin):
class MontecarloTransportSolver(HDFWriterMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the C be capitalized for correct camel case?

@andrewfullard andrewfullard self-requested a review January 10, 2024 15:34
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.

I'll pat myself on the back here

@wkerzendorf wkerzendorf merged commit 3dfcb52 into tardis-sn:master Jan 17, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants