FIX: Make confound expansion column order deterministic#1045
Open
lobennett wants to merge 1 commit into
Open
Conversation
parse_formula and its helpers in interfaces/confounds.py used set()-based iteration in three places, so columns of desc-confounds_timeseries.tsv could shuffle between otherwise-identical fMRIPrep runs even with --skull-strip-fixed-seed and --random-seed set. Replace with order-preserving alternatives: dict.fromkeys for the parse_formula deduplication, an ordered generator for the _expand_shorthand 'others' substitution, and sorted() around the set arithmetic in temporal_derivatives and exponential_terms. Closes nipreps/fmriprep#3501.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes nipreps/fmriprep#3501.
Changes
parse_formula: replacelist(set(reduce(...)))withlist(dict.fromkeys(reduce(...)))to preserve insertion order while deduplicating._expand_shorthand: build theotherssubstitution with an ordered generator overvariablesrather thanset(variables) - set(formula_variables).temporal_derivatives/exponential_terms: wrapset(order) - {0|1}insorted()so multi-order expansions iterate in ascending order.Why
Each of those
set()iterations depends on Python's hash seed, so the same fMRIPrep run could produce columns in different order between executions. The user reported this in nipreps/fmriprep#3501 with--skull-strip-fixed-seed --omp-nthreads 1 --random-seed Nset, expecting bit-identical output across runs.Testing
Added
test_expansion_column_order_deterministicasserting the exact expected column order for(dd1(a + b + c))^^2 + othersagainst the existing test fixture. Empirically:PYTHONHASHSEEDvalues I tried (1, 2, 3, 42, 100, 9999).test_expansion_*tests continue to pass (they assert column sets, not order, so they were unaffected).ruff check,ruff format --check, andcodespellpass on both modified files.