search

tardis-sn / tardis

TARDIS - Temperature And Radiative Diffusion In Supernovae
https://tardis-sn.github.io/tardis
202 stars 405 forks source link

Macroatom restructure #2786

Closed Rodot- closed 2 months ago

Rodot- commented 2 months ago

:pencil: Restructures the MacroAtom to make it more independent from the plasma

Type: :rocket: feature

The properties of the macro atom used for montecarlo/opacity purposes should be decoupled from the plasma, this PR attempts to restructure the macro atom workflow.

:vertical_traffic_light: Testing

How did you test these changes?

:ballot_box_with_check: Checklist

tardis-bot commented 2 months ago

*beep* *bop* Hi human, I ran ruff on the latest commit (537562c1df93c5c4d5d7401f0b25a97a96dac12b). Here are the outputs produced. Results can also be downloaded as artifacts here. Summarised output:

```diff 7 G004 [ ] Logging statement uses f-string 6 I001 [*] Import block is un-sorted or un-formatted 6 D202 [*] No blank lines allowed after function docstring (found 1) 3 F401 [*] `numpy` imported but unused 2 INP001 [ ] File `tardis/opacities/tests/test_opacity_solver.py` is part of an implicit namespace package. Add an `__init__.py`. 2 E722 [ ] Do not use bare `except` 2 UP004 [*] Class `MacroAtomSolver` inherits from `object` 2 UP008 [*] Use `super()` instead of `super(__class__, self)` 1 ANN204 [ ] Missing return type annotation for special method `__getitem__` 1 RET505 [ ] Unnecessary `else` after `return` statement 1 RET506 [ ] Unnecessary `else` after `raise` statement 1 D406 [*] Section name should end with a newline ("Returns") 1 D407 [*] Missing dashed underline after section ("Returns") 1 D411 [*] Missing blank line before section ("Parameters") 1 TRY300 [ ] Consider moving this statement to an `else` block ```

Complete output(might be large):

```diff tardis/opacities/macro_atom/base.py:120:5: D411 [*] Missing blank line before section ("Parameters") tardis/opacities/macro_atom/base.py:227:5: E722 Do not use bare `except` tardis/opacities/macro_atom/base.py:246:14: UP008 Use `super()` instead of `super(__class__, self)` tardis/opacities/macro_atom/base.py:357:9: E722 Do not use bare `except` tardis/opacities/macro_atom/macroatom_solver.py:8:23: UP004 [*] Class `MacroAtomSolver` inherits from `object` tardis/opacities/macro_atom/macroatom_solver.py:14:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/opacities/macro_atom/macroatom_solver.py:28:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/opacities/macro_atom/macroatom_solver.py:94:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/opacities/opacity_solver.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/opacities/opacity_solver.py:11:21: UP004 [*] Class `OpacitySolver` inherits from `object` tardis/opacities/opacity_solver.py:19:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/opacities/opacity_state.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/opacities/opacity_state.py:42:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/opacities/opacity_state.py:174:9: ANN204 Missing return type annotation for special method `__getitem__` tardis/opacities/opacity_state.py:180:9: D407 [*] Missing dashed underline after section ("Returns") tardis/opacities/opacity_state.py:180:9: D406 [*] Section name should end with a newline ("Returns") tardis/opacities/opacity_state.py:215:5: D202 [*] No blank lines allowed after function docstring (found 1) tardis/opacities/tests/test_opacity_solver.py:1:1: INP001 File `tardis/opacities/tests/test_opacity_solver.py` is part of an implicit namespace package. Add an `__init__.py`. tardis/opacities/tests/test_opacity_solver.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/opacities/tests/test_opacity_solver.py:2:17: F401 [*] `numpy` imported but unused tardis/opacities/tests/test_opacity_solver.py:7:44: F401 [*] `tardis.opacities.opacity_state.OpacityState` imported but unused tardis/opacities/tests/test_opacity_solver.py:8:42: F401 [*] `tardis.opacities.tau_sobolev.calculate_sobolev_line_opacity` imported but unused tardis/opacities/tests/test_opacity_state_numba.py:1:1: INP001 File `tardis/opacities/tests/test_opacity_state_numba.py` is part of an implicit namespace package. Add an `__init__.py`. tardis/opacities/tests/test_opacity_state_numba.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/simulation/base.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/simulation/base.py:155:14: UP008 Use `super()` instead of `super(__class__, self)` tardis/simulation/base.py:199:13: RET506 Unnecessary `else` after `raise` statement tardis/simulation/base.py:263:17: G004 Logging statement uses f-string tardis/simulation/base.py:270:9: RET505 Unnecessary `else` after `return` statement tardis/simulation/base.py:451:13: G004 Logging statement uses f-string tardis/simulation/base.py:549:13: G004 Logging statement uses f-string tardis/simulation/base.py:656:25: G004 Logging statement uses f-string tardis/simulation/base.py:659:13: G004 Logging statement uses f-string tardis/simulation/base.py:664:13: G004 Logging statement uses f-string tardis/simulation/base.py:715:13: TRY300 Consider moving this statement to an `else` block tardis/simulation/base.py:717:26: G004 Logging statement uses f-string tardis/transport/montecarlo/base.py:1:1: I001 [*] Import block is un-sorted or un-formatted Found 37 errors. [*] 20 fixable with the `--fix` option (2 hidden fixes can be enabled with the `--unsafe-fixes` option). ```
codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 95.29412% with 4 lines in your changes missing coverage. Please review.

Project coverage is 69.43%. Comparing base (63eb762) to head (537562c). Report is 1 commits behind head on master.

Files Patch % Lines
tardis/opacities/macro_atom/base.py 88.23% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2786 +/- ## ========================================== - Coverage 70.29% 69.43% -0.87% ========================================== Files 203 206 +3 Lines 15255 15509 +254 ========================================== + Hits 10724 10769 +45 - Misses 4531 4740 +209 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

andrewfullard commented 2 months ago

I'm just seeing additions here and no deletions. Is the general idea to duplicate existing functionality?

Rodot- commented 2 months ago

I'm just seeing additions here and no deletions. Is the general idea to duplicate existing functionality?

For the time being, yes, at least until I can guarantee everything works.

tardis-bot commented 2 months ago

*beep* *bop* Hi human, I ran benchmarks as you asked comparing master (2df54e5dd78595aea135c58cf4d97169bbe5694e) and the latest commit (537562c1df93c5c4d5d7401f0b25a97a96dac12b). Here are the logs produced by ASV. Results can also be downloaded as artifacts here.

Significantly changed benchmarks:

```diff | Change | Before [2df54e5d] | After [537562c1] | Ratio | Benchmark (Parameter) | |----------|------------------------------|---------------------|---------|----------------------------------------------------------------------------------------------------------------------| | - | 1.62±0.04μs | 1.21±0μs | 0.75 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary | ```

All benchmarks:

```diff Benchmarks that have improved: | Change | Before [2df54e5d] | After [537562c1] | Ratio | Benchmark (Parameter) | |----------|------------------------------|---------------------|---------|----------------------------------------------------------------------------------------------------------------------| | - | 1.62±0.04μs | 1.21±0μs | 0.75 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary | Benchmarks that have stayed the same: | Change | Before [2df54e5d] | After [537562c1] | Ratio | Benchmark (Parameter) | |----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------| | | 501±100ns | 621±200ns | ~1.24 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation | | | 44.1±20μs | 52.7±20μs | ~1.20 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission | | | 45.7±20μs | 51.5±30μs | ~1.13 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter | | | 3.05±0.6μs | 3.41±0.6μs | ~1.12 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell | | | 561±200ns | 621±200ns | ~1.11 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation | | | 2.81±0.8μs | 3.02±0.6μs | 1.07 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket | | | 6.37±2μs | 6.83±1μs | 1.07 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket | | | 3.45±0ms | 3.65±0.04ms | 1.06 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') | | | 18.8±4μs | 19.8±4μs | 1.05 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list | | | 530±200ns | 551±200ns | 1.04 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation | | | 1.44±0.4μs | 1.50±0.3μs | 1.04 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line | | | 202±0.07ns | 206±0.1ns | 1.02 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body | | | 2.51±0.4ms | 2.56±0.4ms | 1.02 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop | | | 1.66±0ms | 1.68±0.01ms | 1.01 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop | | | 2.07±0m | 2.07±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions | | | 63.2±0.2ms | 63.4±0.04ms | 1.00 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe | | | 39.3±0.05s | 38.9±0.02s | 0.99 | run_tardis.BenchmarkRunTardis.time_run_tardis | | | 1.06±0m | 1.04±0m | 0.99 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking | | | 2.25±1μs | 2.23±2μs | 0.99 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators | | | 31.1±0.08μs | 30.4±0.02μs | 0.98 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list | | | 7.97±2μs | 7.67±2μs | 0.96 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley | | | 2.79±0.01ms | 2.60±0ms | 0.93 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') | | | 770±0.3ns | 719±0.5ns | 0.93 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter | ```

If you want to see the graph of the results, you can check it here