Closed tsalo closed 5 years ago
@jdkent I still need to add tests and update documentation (hence the WIP label), but I wanted to get your thoughts on the method and code before doing all of that.
Merging #204 into master will decrease coverage by
12.54%
. The diff coverage is46.66%
.
@@ Coverage Diff @@
## master #204 +/- ##
===========================================
- Coverage 85.88% 73.33% -12.55%
===========================================
Files 10 10
Lines 425 435 +10
Branches 47 50 +3
===========================================
- Hits 365 319 -46
- Misses 50 101 +51
- Partials 10 15 +5
Merging #204 into master will decrease coverage by
0.07%
. The diff coverage is88.23%
.
@@ Coverage Diff @@
## master #204 +/- ##
==========================================
- Coverage 86.04% 85.97% -0.08%
==========================================
Files 10 10
Lines 430 442 +12
Branches 48 52 +4
==========================================
+ Hits 370 380 +10
- Misses 50 52 +2
Partials 10 10
Hi @tsalo, I'm currently working on reducing a backlog of duties, but I will get to this by next Tuesday at the latest.
Hi @tsalo, sorry I haven't gotten to this yet, I've been working on simulations (using the LSA code you provided, thank you!) and I am presenting on that data next Tuesday. The process has been a little more taxing than I anticipated.
I don't think FIR + LSA would work, because any overlapping timepoints across trials would have duplicate FIR regressors. I haven't tried it though, so maybe it's possible if the delay times are chosen carefully? Still, it's not something I've come across in the literature.
I'm not sure why CI is currently breaking. My latest commit only removed the commented-out line of code and the commit before that was passing.
This may be related to bids-standard/pybids#496, and it was fixed in niworkflows with poldracklab/niworkflows#405, but I'll see if there is something else going on (either instead or in addition to)
Once #212 passes and I merge with master, this branch should be fixed once it is merged with master.
P.S. I'm adding you as a collaborator, so when you have an approving review, you can merge in the pull request yourself. link
Great, work, I think this is pretty much ready. For documentation I would like an entry in workflows.rst showing how this method is different from normal lss
.
Should the description go in the paragraph below the Participant Workflow figure? What about in the Beta Series page?
Good question. I'm imagining that under Beta Series Workflow section header, we will have several subsections; one for plain lss
, one for lss with fir
, and in a separate pull request add one for lsa
. I can do LSA so you can have a model of what I'm thinking.
As for the Beta Series page, we can add a sentence or two under conceptual background.
WDYT?
Sounds good to me. Please let me know what you think of the changes.
Summary
Fixes None. References #197.
Here is an example FS design matrix.
List of changes proposed in this PR (pull-request)
--fir-delays
argument to the CLI and all associated functions.