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

Get line interaction #2780

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

Sumit112192
Copy link
Contributor

@Sumit112192 Sumit112192 commented Aug 6, 2024

📝 Description

Type: 🚀 feature

This PR builds up on PR #2736. Just like boundary interaction this PR introduces the line interaction data to the tracker.

@Sumit112192
Copy link
Contributor Author

I have to implement tests for this new functionality

@tardis-bot
Copy link
Contributor

tardis-bot commented Aug 6, 2024

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

3	I001  	[*] Import block is un-sorted or un-formatted
2	PIE790	[*] Unnecessary `pass` statement
2	D411  	[*] Missing blank line before section ("Parameters")
2	UP004 	[*] Class `RPacketLastInteractionTracker` inherits from `object`
1	B009  	[*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.

Complete output(might be large):

tardis/transport/montecarlo/packet_trackers.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/packet_trackers.py:52:22: UP004 [*] Class `RPacketTracker` inherits from `object`
tardis/transport/montecarlo/packet_trackers.py:55:5: D411 [*] Missing blank line before section ("Parameters")
tardis/transport/montecarlo/packet_trackers.py:273:48: B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.
tardis/transport/montecarlo/packet_trackers.py:293:37: UP004 [*] Class `RPacketLastInteractionTracker` inherits from `object`
tardis/transport/montecarlo/packet_trackers.py:296:5: D411 [*] Missing blank line before section ("Parameters")
tardis/transport/montecarlo/packet_trackers.py:338:9: PIE790 [*] Unnecessary `pass` statement
tardis/transport/montecarlo/packet_trackers.py:345:9: PIE790 [*] Unnecessary `pass` statement
tardis/transport/montecarlo/single_packet_loop.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/tests/test_rpacket_tracker.py:1:1: I001 [*] Import block is un-sorted or un-formatted
Found 10 errors.
[*] 10 fixable with the `--fix` option.

@Sumit112192 Sumit112192 force-pushed the GetLineInteraction branch 2 times, most recently from dfda31c to 52af787 Compare August 10, 2024 17:36
@Sumit112192 Sumit112192 marked this pull request as ready for review August 10, 2024 17:37
@Sumit112192
Copy link
Contributor Author

@wkerzendorf @andrewfullard please review.

@tardis-bot
Copy link
Contributor

tardis-bot commented Aug 10, 2024

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

Significantly changed benchmarks:

| Change   | Before [18dcec87] <master>   | After [ac8189dc]    |   Ratio | Benchmark (Parameter)                                                                                                       |
|----------|------------------------------|---------------------|---------|-----------------------------------------------------------------------------------------------------------------------------|
| +        | 48.3±0.02μs                  | 65.1±0.02μs         |    1.35 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list(100, 50) |
| +        | 7.53±0.01μs                  | 9.32±0.01μs         |    1.24 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list(10, 50)  |

All benchmarks:

Benchmarks that have stayed the same:

| Change   | Before [18dcec87] <master>   | After [ac8189dc]    | Ratio   | Benchmark (Parameter)                                                                                                                        |
|----------|------------------------------|---------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------|
|          | 52.8±0.2μs                   | 67.0±0.02μs         | ~1.27   | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list(100, 10)                  |
|          | 7.23±0.01μs                  | 8.57±0.04μs         | ~1.19   | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list(10, 10)                   |
|          | 18.1±5μs                     | 21.0±6μs            | ~1.16   | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list(10, 10)  |
|          | 521±100ns                    | 581±200ns           | ~1.12   | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation                                               |
|          | 3.44±1μs                     | 3.85±0.6μs          | ~1.12   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell                                       |
|          | 19.7±5μs                     | 21.9±7μs            | ~1.11   | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list(10, 50)  |
|          | 2.87±0.01ms                  | 2.54±0ms            | ~0.89   | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter')                                              |
|          | 1.62±0.4μs                   | 1.38±0.3μs          | ~0.85   | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line                             |
|          | 1.71±0.01ms                  | 1.88±0.01ms         | 1.09    | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop                                      |
|          | 61.6±0.2ms                   | 67.0±0.1ms          | 1.09    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe(100, 10)                  |
|          | 612±100ns                    | 651±100ns           | 1.06    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation                                       |
|          | 1.02±0m                      | 1.08±0m             | 1.06    | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking                                                                               |
|          | 46.5±20μs                    | 49.2±30μs           | 1.06    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter                                                   |
|          | 42.9±20μs                    | 44.8±30μs           | 1.05    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission                                                  |
|          | 2.96±0.6ms                   | 3.08±0.6ms          | 1.04    | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop                                 |
|          | 6.48±1μs                     | 6.77±0.8μs          | 1.04    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket                                                    |
|          | 63.8±0.2ms                   | 65.9±0.2ms          | 1.03    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe(10, 50)                   |
|          | 551±200ns                    | 561±200ns           | 1.02    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation                                         |
|          | 63.7±0.05ms                  | 64.9±0.1ms          | 1.02    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe(10, 10)                   |
|          | 62.8±0.2ms                   | 64.3±0.1ms          | 1.02    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe(100, 50)                  |
|          | 39.6±0.04s                   | 40.0±0.02s          | 1.01    | run_tardis.BenchmarkRunTardis.time_run_tardis                                                                                                |
|          | 2.07±0m                      | 2.07±0m             | 1.00    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions                                          |
|          | 203±0.3ns                    | 202±0.01ns          | 1.00    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body                                                |
|          | 1.20±0μs                     | 1.17±0μs            | 0.98    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary                         |
|          | 742±4ns                      | 729±0.9ns           | 0.98    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter                                                |
|          | 2.90±0.9μs                   | 2.85±0.8μs          | 0.98    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket                                                |
|          | 7.98±2μs                     | 7.64±2μs            | 0.96    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley                                             |
|          | 25.3±5μs                     | 24.1±4μs            | 0.95    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list(100, 50) |
|          | 2.15±2μs                     | 2.03±1μs            | 0.94    | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators                |
|          | 26.1±5μs                     | 24.2±4μs            | 0.93    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list(100, 10) |
|          | 3.88±0ms                     | 3.56±0ms            | 0.92    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom')                                            |

Benchmarks that have got worse:

| Change   | Before [18dcec87] <master>   | After [ac8189dc]    |   Ratio | Benchmark (Parameter)                                                                                                       |
|----------|------------------------------|---------------------|---------|-----------------------------------------------------------------------------------------------------------------------------|
| +        | 48.3±0.02μs                  | 65.1±0.02μs         |    1.35 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list(100, 50) |
| +        | 7.53±0.01μs                  | 9.32±0.01μs         |    1.24 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list(10, 50)  |

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

@Sumit112192
Copy link
Contributor Author

@andrewfullard Can you look at the segregation of line interaction as before line interaction and after line interaction in the track_line_interaction function and suggest some changes or comments?

@andrewfullard
Copy link
Contributor

@andrewfullard Can you look at the segregation of line interaction as before line interaction and after line interaction in the track_line_interaction function and suggest some changes or comments?

Given that you need to detect the line interaction before and after the interaction process, I'm not sure what else can be done apart from essentially duplicating the method instead of having the if else switch.

@Sumit112192 Sumit112192 marked this pull request as draft August 12, 2024 14:28
@Sumit112192 Sumit112192 marked this pull request as ready for review August 14, 2024 12:58
@Sumit112192
Copy link
Contributor Author

The reason rpacket_tracker's boundary_interaction tests are failing because both line_interactions and boundary_interactions have a common field event_id more specifically the interaction number. Since, the line_interactions are added, the event_id of boundary_interactions are changed. So, I have to update the regression data for the boundary_interaction.

@Sumit112192 Sumit112192 marked this pull request as draft August 16, 2024 12:53
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.

3 participants