incf-nidash / nidmresults-fsl

A python library to export FSL's feat results to NIDM-Results
http://nidm.nidash.org/specs/nidm-results.html
MIT License
3 stars 10 forks source link

Unclear error: "Inconsistency: all regressors must have the same type of HRF (3 and 2 found)" #125

Open mih opened 6 years ago

mih commented 6 years ago

In a first attempt to explore nidmfsl I got this error:

% nidmfsl 1stlvl_glm.feat
Exporting NIDM results from /tmp/tmp.U5HOVWLTMP/localizerdemo/1stlvl_glm.feat
Traceback (most recent call last):
  File "/home/mih/env/datalad3-dev/bin/nidmfsl", line 6, in <module>
    exec(compile(open(__file__).read(), __file__, 'exec'))
  File "/home/mih/hacking/nidm/nidmresults-fsl/bin/nidmfsl", line 46, in <module>
    fslnidm.parse()
  File "/home/mih/hacking/nidm/nidmresults-fsl/nidmfsl/fsl_exporter/fsl_exporter.py", line 143, in parse
    super(FSLtoNIDMExporter, self).parse()
  File "/home/mih/hacking/nidm/nidmresults/nidmresults/exporter.py", line 97, in parse
    self.model_fittings = self._find_model_fitting()
  File "/home/mih/hacking/nidm/nidmresults-fsl/nidmfsl/fsl_exporter/fsl_exporter.py", line 185, in _find_model_fitting
    design_matrix = self._get_design_matrix(analysis_dir)
  File "/home/mih/hacking/nidm/nidmresults-fsl/nidmfsl/fsl_exporter/fsl_exporter.py", line 655, in _get_design_matrix
    ' found)')
Exception: Inconsistency: all regressors must have the same type of HRF (3 and 2 found)

It seems that this is a limitation of nidmfsl, and not an error in the analysis, but the exception would allow for both interpretations.

Is it not possible to represent different HRF models in NIDM results, or is this merely a temporary "didn't get to it yet" issue?

nicholst commented 6 years ago

Thanks for letting us know Michael! BTW, what is the use case? I can’t think of anytime that I’ve mixed HRF types, and won’t normally recommend it to anyone.

@cmaumet: is this single-HRF restriction an implementational convenience or part of the NIDM Spec?

mih commented 6 years ago

There is no real use case. I just clicked together a toy analysis for: https://github.com/ReproNim/ohbm2018-training/pull/3 -- but this particular choice was unintentional. The underlying use case is to build a metadata extractor for datalad (https://github.com/datalad/datalad-neuroimaging). We have started working on capturing as much of NIDM as possible, but I completely missed that this library for FSL existed (@yarikoptic pointed it out to me). And for this effort we would naturally aim for something that is as robust as possible given the to-be-expected variability of configurations in the wild. In other words, we cannot really expect sanity as a precondition ;-)

nicholst commented 6 years ago

I think mixing HRFs in one model is an edge case if not bad practice. I'd change your design.fsf to have all fmri(convolveX) values to be the same.

Looking at the spec, HRF is an instance of Convolution Basis Set, which is an attribute of a Design Matrix. (Excuse me if I've mangled the Semantic Web semantics). So I'm afraid that we don't currently allow a column-specific HRF.

There are a constellation of different analysis options and in building NIDM Results we tried to capture the typical ones, but if you think it's important use case, create an [ENH] issue on https://github.com/incf-nidash/nidm-specs/issues

yarikoptic commented 6 years ago

done: https://github.com/incf-nidash/nidm-specs/issues/463

cmaumet commented 6 years ago

@mih, @yarikoptic: thank you for your feedback and thank you @nicholst for your answers! Let's see how we can update NIDM-Results to include this usecase!