bids-standard / stats-models

BIDS Stats Models Specification
https://bids-standard.github.io/stats-models
Other
2 stars 9 forks source link

CHANGE: Update HRF schema with final decision #36

Closed effigies closed 2 years ago

effigies commented 2 years ago

Following discussion about the over-specified HRF, I am proposing specific schematic changes in line with the text @adelavega wrote.

cc @bthirion @nicholst

Remi-Gau commented 2 years ago

makes sense to me actually

nicholst commented 2 years ago

Looks good except, I had to look up exactly what Glover corresponded to (wasn't sure if it was AFNI or SPM). I know this is just inherited from the existing text, but I wonder if we can make it clearer.

By searching the FSL help list, I was able confirm that Glover is FSL (and FSL's double-gamma is supposed to be exactly SPM). Given that AFNI doesn't do Glover as an option (unlike SPM/SPMG2/SPMG3 in 3dDeconvolve) and it's default is slightly different (see e.g. this abstract), should we just change "glover" to "fsl"? Or add text afterwards like "Note that "glover" is the single gamma HRF used in FSL."

(FWIW, I see that nipy/modalities/fmri/hrf.py provides Glover and AFNI's default... tempting me to suggest we should also include "afni" as an option.)

adelavega commented 2 years ago

I think the only reason we went with "glover" was that its what nilearn calls it. I do sort of like "glover" better since its not software specific (at least FSL + nilearn support it), but I agree such a note would be helpful. Include AFNI also seems fine with me.

bthirion commented 2 years ago

I think the only reason we went with "glover" was that its what nilearn calls it. I do sort of like "glover" better since its not software specific (at least FSL + nilearn support it), but I agree such a note would be helpful. Include AFNI also seems fine with me.

I'm with you. In general I'm not very comfortable with referring to model parts through the sofwtare that implements them. I'd rather avoid it whenever this is possible.

effigies commented 2 years ago

Is there a consensus set of changes? Apologies, but I can't tell if there's agreement or disagreement in the comments here.

adelavega commented 2 years ago

I believe we settled on calling "glover" "glover" not "fsl", but adding a note that it's FSL's HRF. "afni" will be called that in lack of a better name, but does not need derivatives etc since its not an option in afni. If we're wrong on that, we can add later.

I accepted Tom's suggestions.

The only possible change then is: is there a better name for "afni" HRF?

nicholst commented 2 years ago

Regarding afni as a name... I'm basing this on an AFNI presentation that lists GAM(p,q) first in the possible list of HRF options, where the 3dDeconvolve documentation describes this as

     'GAM(p,q)'    = 1 parameter gamma variate                         
                         (t/(p*q))^p * exp(p-t/q)                      
                       Defaults: p=8.6 q=0.547 if only 'GAM' is used   
                     ** The peak of 'GAM(p,q)' is at time p*q after    
                        the stimulus.   

Sooooo... we could say "afni-gam" but I don't think it really is necessary. Ultimately it's a placeholder until someone makes a BIDS Models interpreter for AFNI, and to make it clear that using GAM in 3dDeconvolve will not give you glover or spm.

adelavega commented 2 years ago

Okay, I'm fine with going w/ afni. In general I think this controlled vocabulary can be updated in future backwards incompatible versions is needed.

Also FWIW, since FitLins already wraps AFNI's 3dREMLFit it probably wouldn't be much more work to also support native HRF convolution w/ AFNI (but I say this without knowing the details).

effigies commented 2 years ago

Okay. I think everybody's at least not outraged by this. Let's merge and if anybody wants to make further improvements, they're welcome.