-
-
Notifications
You must be signed in to change notification settings - Fork 421
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
Replace M_PI=np.arccos(-1) with M_PI=np.pi #2870
Conversation
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
*beep* *bop* 4 RET505 [ ] Unnecessary `else` after `return` statement
2 E999 [ ] SyntaxError: Expected an expression
1 RET506 [ ] Unnecessary `else` after `raise` statement
1 W292 [*] No newline at end of file
1 F811 [ ] Redefinition of unused `opacity_state_initialize` from line 13
Complete output(might be large): .mailmap:1:38: E999 SyntaxError: Expected an expression
.mailmap:290:39: W292 [*] No newline at end of file
docs/physics/tardisgamma/index.rst:1:1: E999 SyntaxError: Expected an expression
tardis/spectrum/formal_integral.py:23:5: F811 Redefinition of unused `opacity_state_initialize` from line 13
tardis/spectrum/formal_integral.py:340:13: RET506 Unnecessary `else` after `raise` statement
tardis/spectrum/formal_integral.py:681:5: RET505 Unnecessary `else` after `return` statement
tardis/spectrum/formal_integral.py:717:5: RET505 Unnecessary `else` after `return` statement
tardis/spectrum/formal_integral_cuda.py:375:5: RET505 Unnecessary `else` after `return` statement
tardis/spectrum/formal_integral_cuda.py:420:5: RET505 Unnecessary `else` after `return` statement
Found 9 errors.
[*] 1 fixable with the `--fix` option.
|
*beep* *bop* Significantly changed benchmarks: All benchmarks: Benchmarks that have stayed the same:
| Change | Before [a1f621f6] <master> | After [76665954] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
| | 1.36±0.3μs | 1.63±0.2μs | ~1.20 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line |
| | 42.9±30μs | 51.1±30μs | ~1.19 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter |
| | 22.8±6μs | 20.1±4μs | ~0.88 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
| | 612±100ns | 521±200ns | ~0.85 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation |
| | 2.07±0.1ms | 1.67±0.01ms | ~0.81 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
| | 1.20±0μs | 1.31±0μs | 1.09 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary |
| | 2.00±1μs | 2.15±1μs | 1.08 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators |
| | 3.63±0.02ms | 3.78±0.01ms | 1.04 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') |
| | 39.4±0.02s | 40.2±0.07s | 1.02 | run_tardis.BenchmarkRunTardis.time_run_tardis |
| | 64.4±0.07ms | 65.6±0.5ms | 1.02 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe |
| | 7.08±2μs | 7.20±1μs | 1.02 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley |
| | 1.05±0m | 1.06±0m | 1.01 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking |
| | 42.3±20μs | 42.7±20μs | 1.01 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission |
| | 541±100ns | 541±100ns | 1.00 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation |
| | 2.79±0.01ms | 2.80±0.02ms | 1.00 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') |
| | 2.08±0m | 2.08±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions |
| | 204±0.2ns | 205±0.1ns | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body |
| | 3.21±0.6μs | 3.20±0.3μs | 1.00 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell |
| | 721±0.4ns | 713±0.9ns | 0.99 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter |
| | 30.5±0.04μs | 30.2±0.1μs | 0.99 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list |
| | 572±200ns | 561±100ns | 0.98 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation |
| | 2.62±0.4ms | 2.57±0.4ms | 0.98 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop |
| | 2.81±0.5μs | 2.71±0.6μs | 0.97 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket |
| | 6.85±2μs | 6.62±0.7μs | 0.97 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket |
If you want to see the graph of the results, you can check it here |
Can you change the M_PI to PI in this PR? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2870 +/- ##
==========================================
- Coverage 69.97% 69.60% -0.38%
==========================================
Files 216 216
Lines 16077 16077
==========================================
- Hits 11250 11190 -60
- Misses 4827 4887 +60 ☔ 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.
Thank you for your contribution
📝 Description
Type: 🪲
bugfix
This pull request addresses the inconsistent usage of the constant for π (pi) in the code. The previously assigned variable
M_PI
was defined asnp.arccos(-1)
. This has now been replaced withM_PI=np.pi
, which is a more straightforward and standardized way to reference the value of π using NumPy's built-in constant.This change improves code readability and consistency, ensuring that we use
np.pi
across the codebase instead of redefining π in different ways.This PR resolves issue #2863 regarding the inconsistent definition of π.
📌 Resources
🚦 Testing
How did you test these changes?
I ran the testing pipeline, and all tests passed successfully, confirming that the change does not introduce any issues.
☑️ Checklist
build_docs
label