Fix for fir_delays issue in Convolve #1164
Open
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.