Standardize sorting with a global #3223
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR systematically updates all sorting operations throughout the TARDIS codebase to use stable sorting algorithms. The change aims to resolve potential cross-platform differences between Mac and Linux by ensuring consistent ordering of elements with equal sort keys.
- Replaced all instances of
np.argsort(),np.sort(), andDataFrame.sort_*()methods with their stable equivalents - Added
kind="stable"parameter to ensure deterministic sorting behavior across platforms - Also includes minor code formatting improvements (line breaks, string concatenation fixes)
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tardis/workflows/util.py | Updated numpy argsort to use stable sorting |
| tardis/visualization/widgets/line_info.py | Updated pandas sort_values and numpy sort operations to stable, plus code formatting |
| tardis/visualization/widgets/custom_abundance.py | Updated pandas sort_index calls to stable sorting |
| tardis/visualization/tools/sdec_plot.py | Updated pandas sort_values and numpy sort to stable |
| tardis/transport/montecarlo/estimators/util.py | Updated pandas sort_index to stable |
| tardis/plasma/properties/continuum_processes/rates.py | Updated pandas sort_index to stable |
| tardis/plasma/properties/atomic.py | Updated pandas sort_values to stable |
| tardis/plasma/equilibrium/rates/radiative_rates.py | Updated pandas sort_index to stable |
| tardis/plasma/equilibrium/rates/collisional_rates.py | Updated pandas sort_values and sort_index to stable |
| tardis/plasma/equilibrium/rates/collision_strengths.py | Updated pandas sort_index to stable |
| tardis/opacities/macro_atom/transition_probabilities.py | Updated pandas sort_index and sort_values to stable |
| tardis/opacities/macro_atom/macroatom_solver.py | Updated pandas sort_values to stable |
| tardis/model/matter/decay.py | Updated pandas sort_index to stable with code formatting |
| tardis/model/matter/composition.py | Updated pandas sort_index to stable with code formatting |
| tardis/io/tests/test_atomic.py | Updated pandas sort_index to stable |
| tardis/io/model/readers/arepo.py | Updated numpy sort to stable |
| tardis/io/atom_data/base.py | Updated pandas sort_values to stable |
| tardis/gui/widgets.py | Updated list() conversion and code formatting |
| tardis/energy_input/tests/test_energy_source.py | Updated numpy sort to stable |
| tardis/energy_input/samplers.py | Updated numpy sort and argsort to stable with code formatting |
| tardis/energy_input/main_gamma_ray_loop.py | Updated pandas sort_values to stable with code formatting |
| tardis/analysis.py | Updated pandas sort_values and numpy sort to stable |
tardis/energy_input/samplers.py
Outdated
| """ | ||
| energy.sort() | ||
| sorted_indices = np.argsort(energy) | ||
| energy.sort(kind="stable") |
There was a problem hiding this comment.
The function modifies the input energy array in-place with energy.sort(), but then also calls np.argsort(energy) on the already sorted array. This is redundant and potentially incorrect - either sort in-place OR use argsort, not both. The original logic should be preserved.
Contributor
|
*beep* *bop* Details23 I001 [*] unsorted-imports
9 RET505 [*] superfluous-else-return
8 F405 [ ] undefined-local-with-import-star-usage
8 UP008 [ ] super-call-with-parameters
5 G004 [ ] logging-f-string
4 E722 [ ] bare-except
4 F401 [*] unused-import
3 B011 [ ] assert-false
3 C408 [ ] unnecessary-collection-call
3 D202 [*] blank-line-after-function
3 D209 [*] new-line-after-last-paragraph
3 F821 [ ] undefined-name
3 PT015 [ ] pytest-assert-always-false
3 RUF022 [*] unsorted-dunder-all
2 NPY201 [ ] numpy2-deprecation
1 F403 [ ] undefined-local-with-import-star
1 F811 [ ] redefined-while-unused
1 FA100 [ ] future-rewritable-type-annotation
1 FURB187 [ ] list-reverse-copy
1 G001 [ ] logging-string-format
1 ICN001 [ ] unconventional-import-alias
1 PIE790 [*] unnecessary-placeholder
1 RET506 [*] superfluous-else-raise
1 RUF021 [*] parenthesize-chained-operators
1 UP024 [*] os-error-alias
Found 94 errors.
[*] 49 fixable with the `--fix` option (18 hidden fixes can be enabled with the `--unsafe-fixes` option).
Complete output(might be large): Detailstardis/analysis.py:97:9: E722 Do not use bare `except`
tardis/analysis.py:111:9: E722 Do not use bare `except`
tardis/analysis.py:122:9: E722 Do not use bare `except`
tardis/analysis.py:137:9: E722 Do not use bare `except`
tardis/analysis.py:375:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/analysis.py:397:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/analysis.py:422:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/energy_input/main_gamma_ray_loop.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/energy_input/main_gamma_ray_loop.py:31:23: FA100 Add `from __future__ import annotations` to simplify `typing.Optional`
tardis/energy_input/samplers.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/energy_input/samplers.py:155:49: NPY201 `np.trapz` will be removed in NumPy 2.0. Use `numpy.trapezoid` on NumPy 2.0, or ignore this warning on earlier versions.
tardis/energy_input/tests/test_energy_source.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/energy_input/tests/test_energy_source.py:36:5: PT015 Assertion always fails, replace with `pytest.fail()`
tardis/energy_input/tests/test_energy_source.py:36:12: B011 Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`
tardis/energy_input/tests/test_energy_source.py:42:5: PT015 Assertion always fails, replace with `pytest.fail()`
tardis/energy_input/tests/test_energy_source.py:42:12: B011 Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`
tardis/energy_input/tests/test_energy_source.py:48:5: PT015 Assertion always fails, replace with `pytest.fail()`
tardis/energy_input/tests/test_energy_source.py:48:12: B011 Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`
tardis/gui/widgets.py:7:5: I001 [*] Import block is un-sorted or un-formatted
tardis/gui/widgets.py:9:5: I001 [*] Import block is un-sorted or un-formatted
tardis/gui/widgets.py:17:1: I001 [*] Import block is un-sorted or un-formatted
tardis/gui/widgets.py:17:8: ICN001 `matplotlib` should be imported as `mpl`
tardis/gui/widgets.py:18:1: F403 `from matplotlib.figure import *` used; unable to detect undefined names
tardis/gui/widgets.py:25:32: F401 [*] `matplotlib.patches.Circle` imported but unused
tardis/gui/widgets.py:30:30: F401 [*] `tardis.util` imported but unused
tardis/gui/widgets.py:37:9: D202 [*] No blank lines allowed after function docstring (found 1)
tardis/gui/widgets.py:44:23: F405 `Figure` may be undefined, or defined from star imports
tardis/gui/widgets.py:55:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:56:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:59:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:165:27: C408 Unnecessary `dict()` call (rewrite as a literal)
tardis/gui/widgets.py:168:26: C408 Unnecessary `dict()` call (rewrite as a literal)
tardis/gui/widgets.py:169:23: C408 Unnecessary `dict()` call (rewrite as a literal)
tardis/gui/widgets.py:187:9: RET505 [*] Unnecessary `elif` after `return` statement
tardis/gui/widgets.py:199:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:227:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:230:22: F405 `yaml` may be undefined, or defined from star imports
tardis/gui/widgets.py:230:61: F405 `yaml` may be undefined, or defined from star imports
tardis/gui/widgets.py:354:24: F405 `TreeModel` may be undefined, or defined from star imports
tardis/gui/widgets.py:359:38: F405 `TreeDelegate` may be undefined, or defined from star imports
tardis/gui/widgets.py:430:35: UP024 [*] Replace aliased errors with `OSError`
tardis/gui/widgets.py:445:9: PIE790 [*] Unnecessary `pass` statement
tardis/gui/widgets.py:529:9: D202 [*] No blank lines allowed after function docstring (found 1)
tardis/gui/widgets.py:647:41: F405 `np` may be undefined, or defined from star imports
tardis/gui/widgets.py:656:9: D209 [*] Multi-line docstring closing quotes should be on a separate line
tardis/gui/widgets.py:659:41: F405 `np` may be undefined, or defined from star imports
tardis/gui/widgets.py:687:41: F405 `np` may be undefined, or defined from star imports
tardis/gui/widgets.py:814:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:850:9: D209 [*] Multi-line docstring closing quotes should be on a separate line
tardis/gui/widgets.py:926:9: D209 [*] Multi-line docstring closing quotes should be on a separate line
tardis/gui/widgets.py:928:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:1123:14: UP008 Use `super()` instead of `super(__class__, self)`
tardis/gui/widgets.py:1230:9: D202 [*] No blank lines allowed after function docstring (found 1)
tardis/io/atom_data/base.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/io/atom_data/base.py:198:34: G004 Logging statement uses f-string
tardis/io/atom_data/base.py:260:17: G004 Logging statement uses f-string
tardis/io/atom_data/base.py:264:21: G001 Logging statement uses `str.format`
tardis/io/atom_data/base.py:708:17: G004 Logging statement uses f-string
tardis/io/model/readers/arepo.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/model/matter/composition.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/model/matter/composition.py:178:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/model/matter/decay.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/model/matter/decay.py:103:21: G004 Logging statement uses f-string
tardis/opacities/macro_atom/macroatom_solver.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/opacities/macro_atom/transition_probabilities.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/opacities/macro_atom/transition_probabilities.py:16:11: RUF022 [*] `__all__` is not sorted
tardis/plasma/equilibrium/rates/collision_strengths.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/plasma/equilibrium/rates/collisional_rates.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/plasma/equilibrium/rates/radiative_rates.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/plasma/properties/atomic.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/plasma/properties/atomic.py:28:11: RUF022 [*] `__all__` is not sorted
tardis/plasma/properties/atomic.py:555:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/plasma/properties/atomic.py:586:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/plasma/properties/atomic.py:599:17: G004 Logging statement uses f-string
tardis/plasma/properties/atomic.py:640:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/plasma/properties/continuum_processes/rates.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/plasma/properties/continuum_processes/rates.py:23:11: RUF022 [*] `__all__` is not sorted
tardis/plasma/properties/continuum_processes/rates.py:157:13: FURB187 Use of assignment of `reversed` on list `idx_arrays`
tardis/transport/montecarlo/estimators/util.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/estimators/util.py:66:32: NPY201 `np.trapz` will be removed in NumPy 2.0. Use `numpy.trapezoid` on NumPy 2.0, or ignore this warning on earlier versions.
tardis/visualization/tools/sdec_plot.py:8:1: I001 [*] Import block is un-sorted or un-formatted
tardis/visualization/widgets/custom_abundance.py:3:1: I001 [*] Import block is un-sorted or un-formatted
tardis/visualization/widgets/custom_abundance.py:741:16: RUF021 [*] Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
tardis/visualization/widgets/custom_abundance.py:1131:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/visualization/widgets/custom_abundance.py:1422:9: RET506 [*] Unnecessary `else` after `raise` statement
tardis/visualization/widgets/custom_abundance.py:1725:9: F811 Redefinition of unused `input_d_time_0_eventhandler` from line 1714
tardis/visualization/widgets/custom_abundance.py:1755:13: F821 Undefined name `display`
tardis/visualization/widgets/custom_abundance.py:1757:13: F821 Undefined name `display`
tardis/visualization/widgets/custom_abundance.py:1759:13: F821 Undefined name `display`
tardis/visualization/widgets/line_info.py:3:1: I001 [*] Import block is un-sorted or un-formatted
tardis/workflows/tests/test_workflows.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/workflows/tests/test_workflows.py:2:21: F401 [*] `pathlib.Path` imported but unused
tardis/workflows/tests/test_workflows.py:7:51: F401 [*] `tardis.io.configuration.config_reader.Configuration` imported but unused
tardis/workflows/util.py:1:1: I001 [*] Import block is un-sorted or un-formatted
Found 94 errors.
[*] 49 fixable with the `--fix` option (18 hidden fixes can be enabled with the `--unsafe-fixes` option).
|
Contributor
|
*beep* *bop* Hi, human. The Click here to see your results. |
jvshields
force-pushed
the
use_stable_sort
branch
from
6967174 to
95187b9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3223 +/- ##
==========================================
- Coverage 67.96% 66.78% -1.18%
==========================================
Files 172 173 +1
Lines 13211 13230 +19
==========================================
- Hits 8979 8836 -143
- Misses 4232 4394 +162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
*beep* *bop* Significantly changed benchmarks: DetailsAll benchmarks: DetailsBenchmarks that have stayed the same:
| Change | Before [5d6a32e3] <master> | After [283e9cc5] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
| | 551±200ns | 812±100ns | ~1.47 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation |
| | 41.4±30μs | 58.3±20μs | ~1.41 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission |
| | 45.5±20μs | 57.1±20μs | ~1.26 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter |
| | 1.28±0.4μs | 1.44±0.2μs | ~1.12 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line |
| | 6.55±3μs | 7.22±3μs | ~1.10 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley |
| | 29.2±8μs | 26.2±7μs | ~0.90 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
| | 642±200ns | 571±100ns | ~0.89 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation |
| | 3.42±0.4μs | 2.91±0.3μs | ~0.85 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket |
| | 58.2±0.03s | 1.07±0m | 1.10 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking |
| | 5.97±0.5μs | 6.48±0.9μs | 1.09 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket |
| | 4.65±0.04ms | 4.98±0.03ms | 1.07 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') |
| | 2.82±0.5μs | 3.03±0.3μs | 1.07 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell |
| | 58.2±0.1ms | 60.3±0.2ms | 1.04 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe |
| | 1.16±0.01μs | 1.19±0μs | 1.03 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary |
| | 36.5±0.1s | 37.2±0.04s | 1.02 | run_tardis.BenchmarkRunTardis.time_run_tardis |
| | 657±0.2ns | 667±1ns | 1.02 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter |
| | 2.72±0.3ms | 2.76±0.3ms | 1.02 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop |
| | 1.05±0m | 1.05±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions |
| | 1.14±0μs | 1.14±0.02μs | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body |
| | 2.53±0ms | 2.44±0.01ms | 0.96 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
| | 40.1±0.09μs | 38.1±0.01μs | 0.95 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list |
| | 2.08±1μs | 1.95±0.9μs | 0.94 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators |
| | 541±100ns | 501±100ns | 0.93 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation |
| | 3.35±0.03ms | 3.05±0.01ms | 0.91 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') |
If you want to see the graph of the results, you can check it here |
atharva-2001
added
the
run-regression-comparison
Contributor
Author
|
I think we expect this to change the regression data, but probably should be regenerated for this if we're happy with it for more consistency between platforms. |
Contributor
Regression Data Comparison ResultsThe comparison results have been generated and can be viewed here: 📊 View Comparison PlotsmacOS (not continuum)
macOS (continuum)
Ubuntu (not continuum)
Ubuntu (continuum)
You can also download the artifacts directly from the Actions tab. |
jvshields
added
run-regression-comparison
jvshields
commented
Sep 2, 2025
| index = plasma.atomic_data.lines.nu.index | ||
| freqs = plasma.atomic_data.lines.nu.values * u.Hz | ||
| order = np.argsort(freqs, kind="stable") | ||
| order = np.argsort(freqs, kind=SORTING_ALGORITHM) |
Contributor
Author
There was a problem hiding this comment.
This is the one change that breaks the test, because it's the one place stable was applied.
DeerWhale
approved these changes
Sep 2, 2025
andrewfullard
approved these changes
Sep 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes everywhere in tardis use the same sorting algorithm set as a global. Everywhere was using quicksort by default which we don't really want, but this enforces that everywhere so we can make another pr that changes everywhere to stable so we can see those differences.
The regression data in the PR below makes the v_inner_workflow tests pass for this PR (at least on mac) by generating regression data from this PR.
tardis-sn/tardis-regression-data#72