Skip to content

Conversation

@jvshields
Copy link
Contributor

Should fix the cuda references that were changed in #3265

Copilot AI review requested due to automatic review settings September 17, 2025 19:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes references to formal integral CUDA functions that were changed in #3265, updating function names from calculate_z_cuda to calculate_intersection_point_cuda and populate_z_cuda to populate_intersection_points_cuda.

  • Renamed CUDA functions and their corresponding test callers to use more descriptive names
  • Updated variable names throughout the codebase to use more meaningful identifiers (e.g., z to intersection_point)
  • Removed the test_trapezoid_integration_cuda test and its caller function

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test_cuda_formal_integral.py Updates test function names and removes trapezoid integration test
formal_integral_cuda.py Renames CUDA functions and updates parameter/variable names for clarity

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tardis-bot
Copy link
Contributor

tardis-bot commented Sep 17, 2025

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

Details
4	I001  	[*] unsorted-imports
3	F401  	[*] unused-import
2	RET505	[*] superfluous-else-return
1	W291  	[*] trailing-whitespace
1	F811  	[*] redefined-while-unused
Found 11 errors.
[*] 11 fixable with the `--fix` option.

Complete output(might be large):

Details
tardis/spectrum/formal_integral/formal_integral_cuda.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/spectrum/formal_integral/formal_integral_cuda.py:96:5: RET505 [*] Unnecessary `else` after `return` statement
tardis/spectrum/formal_integral/formal_integral_cuda.py:140:5: RET505 [*] Unnecessary `else` after `return` statement
Found 3 errors.
[*] 3 fixable with the `--fix` option.

andrewfullard
andrewfullard previously approved these changes Sep 17, 2025
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'm happy with it. May be worth updating the variable names in these functions to match the changes in other files, but at least it works now.

@jvshields
Copy link
Contributor Author

I'm happy with it. May be worth updating the variable names in these functions to match the changes in other files, but at least it works now.

I did try to do exactly that (all the z -> intersection point and p -> impact_parameter changes), but might not have gotten all the renames from that PR.

wkerzendorf
wkerzendorf previously approved these changes Sep 17, 2025
Copy link
Contributor

@hmperk18 hmperk18 left a comment

Choose a reason for hiding this comment

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

I realize this might not have been the PR to rename everything, if this is that PR, then I can continue to mark missed renames if you wish. For now, I'll just leave it with these 3 commments

@tardis-bot
Copy link
Contributor

tardis-bot commented Sep 17, 2025

*beep* *bop*
Hi human,
I ran benchmarks as you asked comparing master (3967175) and the latest commit (b3497e7).
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 [0a0cfdc8] <master>   | After [b3497e7b]    | Ratio   | Benchmark (Parameter)                                                                                                               |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
|          | 2.98±0.6μs                   | 3.72±6μs            | ~1.25   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket                                       |
|          | 571±200ns                    | 631±200ns           | ~1.11   | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation                              |
|          | 7.08±3μs                     | 7.83±2μs            | ~1.11   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley                                    |
|          | 562±200ns                    | 491±200ns           | ~0.87   | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation                                |
|          | 1.88±0.8μs                   | 1.31±0.4μs          | ~0.70   | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line                    |
|          | 1.97±2μs                     | 2.17±2μs            | 1.10    | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators       |
|          | 43.8±30μs                    | 48.1±30μs           | 1.10    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter                                          |
|          | 3.05±0.5μs                   | 3.33±0.4μs          | 1.09    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell                              |
|          | 471±200ns                    | 491±200ns           | 1.04    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation                                      |
|          | 36.3±0.09s                   | 37.2±0.3s           | 1.03    | run_tardis.BenchmarkRunTardis.time_run_tardis                                                                                       |
|          | 50.8±0.5ms                   | 52.2±0.8ms          | 1.03    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe                  |
|          | 3.63±0.02ms                  | 3.70±0.04ms         | 1.02    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom')                                   |
|          | 1.09±0μs                     | 1.12±0μs            | 1.02    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body                                       |
|          | 663±0.7ns                    | 674±6ns             | 1.02    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter                                       |
|          | 36.6±0.01μs                  | 37.2±0.06μs         | 1.02    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list                  |
|          | 1.13±0μs                     | 1.14±0μs            | 1.01    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary                |
|          | 2.76±0ms                     | 2.76±0ms            | 1.00    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter')                                     |
|          | 1.03±0m                      | 1.03±0m             | 1.00    | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking                                                                      |
|          | 1.02±0m                      | 1.02±0m             | 1.00    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions                                 |
|          | 2.56±0.4ms                   | 2.56±0.4ms          | 1.00    | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop                        |
|          | 42.7±20μs                    | 42.0±20μs           | 0.98    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission                                         |
|          | 2.51±0.08ms                  | 2.41±0.02ms         | 0.96    | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop                             |
|          | 6.77±1μs                     | 6.35±1μs            | 0.94    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket                                           |
|          | 24.2±6μs                     | 22.1±6μs            | 0.91    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |

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

@jvshields jvshields dismissed stale reviews from wkerzendorf and andrewfullard via 6444e67 September 18, 2025 17:25

@cuda.jit
def cuda_vector_integrator(luminosity_density, I_nu, N, radius_max):
def cuda_vector_integrator(luminosity_density, intensities_nu, N, radius_max):
Copy link
Contributor

Choose a reason for hiding this comment

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

N can probably get a rename too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #3310

@codecov
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

❌ Patch coverage is 6.15385% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.26%. Comparing base (3967175) to head (b3497e7).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...s/spectrum/formal_integral/formal_integral_cuda.py 6.15% 61 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3308      +/-   ##
==========================================
- Coverage   68.51%   68.26%   -0.26%     
==========================================
  Files         175      175              
  Lines       13385    13385              
==========================================
- Hits         9171     9137      -34     
- Misses       4214     4248      +34     

☔ 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.

@andrewfullard andrewfullard self-requested a review September 22, 2025 14:04
@jvshields jvshields merged commit 6802bfa into tardis-sn:master Sep 22, 2025
24 of 26 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.

5 participants