Fix for fir_delays issue in Convolve - #1164
Open
jmumford wants to merge 3 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1164 +/- ##
==========================================
- Coverage 89.33% 89.25% -0.09%
==========================================
Files 66 66
Lines 7259 7268 +9
Branches 1149 1152 +3
==========================================
+ Hits 6485 6487 +2
- Misses 565 570 +5
- Partials 209 211 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Bah! Something is off (other than my missing tests). Michael tested this and found it was including a lag0 by default (even though that wasn't specified), so I'll look into that issue next. |
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
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.
Overview
When users correctly specify
fir_delays(FIRDelays in a model spec), they provide delays in TR units. However, incompute_regressorwithin theConvolveclass, the resulting FIR regressors were constructed incorrectly, which showed up as unexpected temporal shifts in the regressors.This happens because
compute_regressorassumes that the providedframe_timesare defined at the same temporal resolution as thefir_delaysspecification (typically TR units). Inpybids,frame_timesis set toresample_frames, which may have a different resolution than the TR. As a result, the FIR shifts were misspecified, and the regressors did not reflect the intended delays.Changes
fir_delays_resample_adjustedand use it incompute_regressorto correctly align delays with the resampled frame times.compute_regressor(issue caused by Change 1). The names embed the delay value input (fir_delays_resample_adjusted), so the resampled delays are replaced with the originalfir_delaysto ensure consistency with model/contrast specification.Things an expert should review closely
tris always available viatr = var.run_info[0].tr. I’m fairly confident this is the case, but it may be worth confirming.resample_framescontains uniformly spaced frame times. This may not hold for inputs that are notSparseRunVariableobjects. This matters because the sampling interval is computed asdt = resample_frames[1] - resample_frames[0], and any non-uniform spacing could affect the FIR delay adjustment.