Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

[FIX] Design matrices are wrong if multiple events in the same regressor have the same onset or offset #442

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mwegrzyn
Copy link

Dear nistats team,

using first_level_model.make_first_level_design_matrix() I noticed that in some cases the baseline of a regressor can shift and never return to zero, instead being stuck at another value (e.g. -1, +1 etc.). I think the issue is that in the function hemodynamic_models._sample_condition() each onset (e.g. a stick function of +1) has to be balanced by an offset of the same, but negative value (e.g. -1). However, if multiple events start at the same time (+1) but end at different times (-1 + -1 = -2), the baseline will permanently shift downwards. The opposite effect occurs when different onsets have the same offset (then the baseline will be permanently shifted in the positive direction). For some examples, including a possible solution, please see the following notebook:

https://gist.github.com/mwegrzyn/80a382f30260f5982e8b5dc5c89867fb

I hope that the suggested pull request is helpful. It's my first one ever, so please let me know if something is not quite ok. Thank you very much!

Best,
Martin

@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #442 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
+ Coverage   92.71%   92.71%   +<.01%     
==========================================
  Files          40       40              
  Lines        4707     4709       +2     
==========================================
+ Hits         4364     4366       +2     
  Misses        343      343
Impacted Files Coverage Δ
nistats/hemodynamic_models.py 98.11% <100%> (+0.03%) ⬆️

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 7b17e51...7f600e7. Read the comment docs.

@bthirion
Copy link
Member

The diagnosis is right, thx.

@bthirion
Copy link
Member

Thx for the fix. An important thing to add is a test that breaks with the previous code and passes with the fix. That would be awesome.
Thx a lot.

@kchawla-pi
Copy link
Collaborator

Hi @mwegrzyn !
thanks for this! We have incorporated all of Nilstats functionality into Nilearn's master and if you are still interested, i would urge you to reopen this PR there.

We are archiving this repo, so it is read-only.

We deeply appreciate your contribution, and I'm hoping to see you on Nilearn!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants