Skip to content

Conversation

@connor-mcclellan
Copy link
Contributor

📝 Description

Type: 🪲 bugfix

The LastInteractionTracker was mistakenly preserving line IDs from previous line interactions, even if the last interaction a packet underwent was not a line interaction. This is expected to produce excess luminosity in things like the SDEC plot and has been responsible for the failure of several regression data comparison tests in #3278.

Regression data will need to be regenerated for tests that compare against a total luminosity or spectrum --- anything summed up by filtering packet data using line IDs from the LastInteractionTracker.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

@tardis-bot
Copy link
Contributor

tardis-bot commented Dec 9, 2025

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

Details

Complete output(might be large):

Details
All checks passed!

@tardis-bot
Copy link
Contributor

tardis-bot commented Dec 9, 2025

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

Significantly changed benchmarks:

Details

All benchmarks:

Details
Benchmarks that have stayed the same:

| Change   | Before [3b071d1f] <master>   | After [dbef2223]    | Ratio   | Benchmark (Parameter)                                                                                                               |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
|          | 27.1±7μs                     | 36.7±10μs           | ~1.35   | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
|          | 7.17±0.02μs                  | 8.84±1μs            | ~1.23   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley                                    |
|          | 5.14±0.1ms                   | 3.93±0.04ms         | ~0.77   | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom')                                   |
|          | 20.1±0.05μs                  | 21.9±0.1μs          | 1.09    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission                                         |
|          | 62.2±0.3ms                   | 66.4±0.3ms          | 1.07    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe                  |
|          | 1.63±0.02ms                  | 1.71±0.01ms         | 1.05    | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop                             |
|          | 551±100ns                    | 571±200ns           | 1.04    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation                              |
|          | 2.92±0.05ms                  | 3.04±0.02ms         | 1.04    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter')                                     |
|          | 1.08±0μs                     | 1.12±0μs            | 1.03    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body                                       |
|          | 501±200ns                    | 511±200ns           | 1.02    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation                                |
|          | 39.1±0.02s                   | 39.8±0.02s          | 1.02    | run_tardis.BenchmarkRunTardis.time_run_tardis                                                                                       |
|          | 59.0±0.08s                   | 59.5±0.3s           | 1.01    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions                                 |
|          | 1.23±0μs                     | 1.24±0μs            | 1.01    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary                |
|          | 700±0.3ns                    | 710±1ns             | 1.01    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter                                       |
|          | 36.5±0.02μs                  | 36.9±0.3μs          | 1.01    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list                  |
|          | 1.10±0m                      | 1.10±0m             | 1.00    | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking                                                                      |
|          | 2.85±0.8ms                   | 2.81±0.8ms          | 0.99    | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop                        |
|          | 3.51±0.01μs                  | 3.47±0μs            | 0.99    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket                                           |
|          | 1.69±0μs                     | 1.67±0μs            | 0.99    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell                              |
|          | 1.84±0.3μs                   | 1.79±0.5μs          | 0.97    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line                    |
|          | 1.55±0μs                     | 1.51±0μs            | 0.97    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket                                       |
|          | 2.62±0.9μs                   | 2.43±1μs            | 0.93    | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators       |
|          | 521±200ns                    | 481±200ns           | 0.92    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation                                      |
|          | 61.1±20μs                    | 55.7±20μs           | 0.91    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter                                          |

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

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.

Looks good, please generate regression data and link the PR

@connor-mcclellan
Copy link
Contributor Author

Looks good, please generate regression data and link the PR

tardis-sn/tardis-regression-data#81

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.

Approved pending tests passing

@connor-mcclellan connor-mcclellan added the git-lfs-pull Allow git lfs pull in PRs label Dec 10, 2025
@github-actions github-actions bot removed the git-lfs-pull Allow git lfs pull in PRs label Dec 10, 2025
@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.43%. Comparing base (6025bed) to head (dbef222).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...transport/montecarlo/packets/packet_collections.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3353      +/-   ##
==========================================
- Coverage   68.70%   68.43%   -0.27%     
==========================================
  Files         177      177              
  Lines       13567    13570       +3     
==========================================
- Hits         9321     9287      -34     
- Misses       4246     4283      +37     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wkerzendorf wkerzendorf merged commit 73b41fc into tardis-sn:master Dec 10, 2025
35 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants