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_regressor
within theConvolve
class, the resulting FIR regressors were constructed incorrectly, which showed up as unexpected temporal shifts in the regressors.This happens because
compute_regressor
assumes that the providedframe_times
are defined at the same temporal resolution as thefir_delays
specification (typically TR units). Inpybids
,frame_times
is 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_adjusted
and use it incompute_regressor
to 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_delays
to ensure consistency with model/contrast specification.Things an expert should review closely
tr
is always available viatr = var.run_info[0].tr
. I’m fairly confident this is the case, but it may be worth confirming.resample_frames
contains uniformly spaced frame times. This may not hold for inputs that are notSparseRunVariable
objects. 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.