Add fixed effects - FEMA contrasts

[adelavega]

This PR depends on: nilearn/nistats#386

• This adds fixed effects contrasts for "FEMA" contrasts in the BIDS StatsModel.
• Pass-through for cases where there is only a single effect/variance per subject. This works automatically since nistats fixed effects can accept one input. Fixes Handle subject level when there is only 1 run #170
• nistats fixed effects does not output p or z values, because that work required the degrees of freedom from the previous level. we might want to add this in the future, but for now, to deal with this, made these two optional throughout the workflow (e.g. in collate functions)

[pep8speaks]

Hello @adelavega, Thank you for updating!

• In the file fitlins/interfaces/nistats.py:

Line 138:15: E126 continuation line over-indented for hanging indent
Line 236:21: E123 closing bracket does not match indentation of opening bracket's line
Line 242:21: E123 closing bracket does not match indentation of opening bracket's line
Line 248:21: E123 closing bracket does not match indentation of opening bracket's line

To test for issues locally, pip install flake8 and then run flake8 fitlins.

Comment last updated at 2019-12-11 05:41:06 UTC

[adelavega]

@effigies no rush on reviewing this, but unless nistats api changes, this looks good to me.

the main point of contention is how to handle the fact that z and p stats are not output by fixed effects.

I also hard coded fixed effects at the subject level. maybe you will disagree with this? or it can be specified in the CLI? In any case this should be temporary until the StatsModel catches up.

[adelavega]

The other point of contention is that nistats fixed effects won't handle smoothing. we can do it ourselves using the masker object (and pass one in), or give a warning. currently, only silently ignoring.

[effigies]

So, this will break (in a new and different way) the ds114 test-retest, where one session is contrasted against another. So at the very least we should condition it on being a pass-through intercept.

Should it also apply to session? Seems weird to do a random-effects combination at session, and then a fixed-effects at subject.

I think I would prefer that we go ahead and put this into the spec, and use the FEMA term. That way existing models continue behaving the same, and there's a way to explicitly specify fixed effects.

For smoothing, I would be inclined to use nilearn tools if reasonable, since nistats generally handles it internally.

Anyway some review comments while I'm here.

[effigies]

Don't all of these get lowercased by pybids?

Suggested change

	if self.inputs.level == 'Subject':
	if self.inputs.level == 'subject':

[adelavega]

This is coming from model_dict so no, its uppercase. I'm getting this value from the for loop used in workflow creation.

[adelavega]

I original had it lowercase and it didnt work.

[adelavega]

Yes, I think this could also be done at the session level...

I would also like to put it in the spec for future reproducibility, but @tyarkoni indicated this might not happen for a while, so he suggested hard-coding it for now.

About smoothing, we could very easily do what nistats does internally. They use the NiftiMasker object. We would simply have to initialize one (with the smoothing parameters), and then transform the images. The reason that Bertrand doesn't make to support it in fixed effects, is he said it would not make sense to do at the subject level so he would rather discourage it heavily. We could follow a similar principle, or leave it up to the user.
See: nilearn/nistats#386 (comment)

[adelavega]

The ds114 test-retest is not part of the CircleCI tests, right? Should it be?

[effigies]

@tyarkoni indicated this might not happen for a while, so he suggested hard-coding it for now.

Can we not just do it? It's a draft spec. If it changes in the future, we adapt.

[effigies]

And yeah, we can add ds114, if we upload a preprocessed copy of the data somewhere. I'd prefer with datalad, but whatever.

[adelavega]

I can host ds114 on our tacc servers

[codecov-io]

Codecov Report

Merging #191 into master will increase coverage by 0.57%.
The diff coverage is 86.84%.

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   76.48%   77.05%   +0.57%     
==========================================
  Files          18       18              
  Lines        1029     1046      +17     
  Branches      181      189       +8     
==========================================
+ Hits          787      806      +19     
  Misses        150      150              
+ Partials       92       90       -2

Flag	Coverage Δ
#ds003	77.05% <86.84%> (+0.57%)	⬆️

Impacted Files	Coverage Δ
fitlins/interfaces/utils.py	83.07% <75%> (+0.53%)	⬆️
fitlins/interfaces/nistats.py	82.89% <92.3%> (+2.6%)	⬆️
fitlins/interfaces/bids.py	73.33% <0%> (+0.37%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad0a153...8c054f3. Read the comment docs.

[adelavega]

Allright, I've updated this branch to use: bids-standard/pybids#520

It now handles reading the "FEMA" contrast type from the bids model.
I tested it on two models in two datasets and its seemed to work fine.
Passthrough at the Subject level using "FEMA" with only a single run per subject also worked.

(We also have a PR on Neuroscout to make these models)

So all we really need now is to hammer out if my proposed changes to the spec draft are OK.
No rush to review this @effigies, but its ready.

cc: @tyarkoni when you get a chance, can you review the changes to the draft spec on google docs? there's no real urgency though.

[effigies]

Cancelling the job because it's broken.

[adelavega]

@effigies if tests pass, this is ready for a final review.

To explain a bit more, previously we were computing a separate l2 model for each contrast, but this is not necessary.

Instead you can make a dummy coded design matrix that represents the identity of each effect_map.

effect_file	cond1	cond2
cond1_effect_size.nii.gz	1	0
cond2_effect_size.nii.gz	0	1
cond1_effect_size.nii.gz	1	0
cond2_effect_size.nii.gz	0	1

You then just translate the weights (represented as dicts), to match the columns:

{name='cond1', weights=[1, 0], type='t'}

This works for F-tests too:

{name='omnibus', weights=[[1, 0], [0, 1]], type='F'}

The weights argument is passed to nistats as second_level_contrast

[adelavega]

Realized that the same logic was not being applied for FEMA contrasts. For those, it's typically a Dummy contrast, but in theory it could be more than that, something like: weights: [1, 1, 0, 0].

That is, you could do a meta-analysis across multiple effects. Maybe its rare, but its not forbidden.

So I added some (admittedly convoluted) logic to index the input effect and variance files based on the contrast weights. If you see a more elegant way to do it, feel free to suggest (but I'm trying to work with the contrast of making the prepare_contrasts function work for both level 1 and 2).

[adelavega]

ping @effigies

[effigies]

Suggested change

	if any([c['type'] != 'FEMA' for c in self.inputs.contrast_info]):
	if any(c['type'] != 'FEMA' for c in self.inputs.contrast_info):

[adelavega]

What happened to the ignore argument in the LoadBIDSModel input spec?

   load_entry_point('neuroscout-cli', 'console_scripts', 'neuroscout')()
  File "/src/neuroscout/neuroscout_cli/cli.py", line 61, in main
    command(deepcopy(args)).run()
  File "/src/neuroscout/neuroscout_cli/commands/run.py", line 57, in run
    retcode = run_fitlins(fitlins_args)
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/fitlins/cli/run.py", line 236, in run_fitlins
    smoothing=opts.smoothing, drop_missing=opts.drop_missing,
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/fitlins/workflows/base.py", line 43, in init_fitlins_wf
    loader.inputs.ignore = ignore
traits.trait_errors.TraitError: Cannot set the undefined 'ignore' attribute of a 'LoadBIDSModelInputSpec' object.

[effigies]

It's now part of the initial layout build.

[adelavega]

Okay, tested this with #202 merged in, and my model is running!

This is ready to merge as far as I'm concerned.

[effigies]

Assuming the setup.cfg is more recent...

[effigies]

Can't commit for you. Feel free to merge once the dependency specifications are made consistent.

[adelavega]

Will do. I'm gonna have to bug to do another release soon though!