-
-
Notifications
You must be signed in to change notification settings - Fork 422
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/plasma] Cleanup plasma assembly #2748
[restructure/plasma] Cleanup plasma assembly #2748
Conversation
…ace.py, transport_montecarlo_numba_interface.py, conftest.py, formal_integral.py, base.py, and macro_atom.py
Please merge master and resolve merge conflicts so I can review this |
…ble_plasma_cleanup
*beep* *bop* Significantly changed benchmarks: All benchmarks: Benchmarks that have stayed the same:
| Change | Before [63eb7625] <master> | After [573b5e5c] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
| | 3.62±0.01ms | 4.40±0.3ms | ~1.22 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') |
| | 2.79±0.6μs | 3.39±0.5μs | ~1.22 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket |
| | 51.9±20μs | 59.6±10μs | ~1.15 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission |
| | 561±200ns | 501±200ns | ~0.89 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation |
| | 3.57±0.9ms | 2.97±0.7ms | ~0.83 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop |
| | 735±0.6ns | 761±1ns | 1.04 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter |
| | 1.69±0.01ms | 1.75±0ms | 1.04 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
| | 38.4±0s | 39.1±0.02s | 1.02 | run_tardis.BenchmarkRunTardis.time_run_tardis |
| | 30.5±0μs | 31.1±0.01μs | 1.02 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list |
| | 7.60±2μs | 7.76±2μs | 1.02 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley |
| | 45.2±20μs | 45.8±30μs | 1.01 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter |
| | 541±200ns | 541±200ns | 1.00 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation |
| | 2.71±0.01ms | 2.72±0ms | 1.00 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') |
| | 1.05±0m | 1.05±0m | 1.00 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking |
| | 2.07±0m | 2.07±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions |
| | 63.4±0.05ms | 62.6±0.2ms | 0.99 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe |
| | 211±1ns | 207±0.02ns | 0.98 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body |
| | 20.1±4μs | 19.7±4μs | 0.98 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
| | 3.50±0.7μs | 3.37±0.5μs | 0.96 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell |
| | 1.52±0.3μs | 1.44±0.5μs | 0.95 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line |
| | 6.96±1μs | 6.60±1μs | 0.95 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket |
| | 2.27±2μs | 2.13±1μs | 0.94 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators |
| | 581±200ns | 541±200ns | 0.93 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation |
| | 1.25±0.01μs | 1.15±0μs | 0.92 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary |
If you want to see the graph of the results, you can check it here |
Co-authored-by: Atharva Arya <[email protected]>
…ble_plasma_cleanup
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2748 +/- ##
==========================================
- Coverage 70.29% 69.32% -0.98%
==========================================
Files 203 205 +2
Lines 15255 15440 +185
==========================================
- Hits 10724 10704 -20
- Misses 4531 4736 +205 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some additional tests/docs may be nice but I know we don't usually have time for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be useful documentation? Not sure what "SCRUN MAGIC" is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it
NLTEPopulationSolverLU, | ||
NLTEPopulationSolverRoot, | ||
) | ||
from tardis.plasma.properties.property_collections import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, could we import property_collections as pc
or something like that? This is a large file and it isn't immediately clear where the variables come from all the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I said - I'm in the process of rewriting large chunks of this.
|
||
|
||
class PlasmaSolverFactory: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class needs docstrings
|
||
""" | ||
atomic_numbers = simulation_state.abundance.index | ||
plasma_solver_factory = PlasmaSolverFactory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the config really need to be passed twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will change in the future - likely. I'm just rewriting all of it.
Stardis tests are also failing, should @jvshields be looped in? |
no - @jvshields is not going to be involved until the plasma has settled. |
Refactor the `PlasmaSolverFactory` class in `tardis/plasma/assembly/base.py` to improve code organization and readability. Remove unused imports and reformat the code using the black linter. Also, remove the unnecessary code block in the Jupyter notebook `docs/physics/plasma/construction_simple_plasma.ipynb`. These changes aim to enhance the maintainability and clarity of the codebase.
…ble_plasma_cleanup
another cleanup PR. Requires merging of #2675