ENH: Enable models for sparsely sampled fMRI series

[effigies]

Closes #252.

[codecov[bot]]

Codecov Report

Merging #376 into master will decrease coverage by 28.01%. The diff coverage is 19.23%.

@@             Coverage Diff             @@
##           master     #376       +/-   ##
===========================================
- Coverage   73.79%   45.78%   -28.02%     
===========================================
  Files          23       23               
  Lines        2492     2514       +22     
  Branches      621      628        +7     
===========================================
- Hits         1839     1151      -688     
- Misses        471     1240      +769     
+ Partials      182      123       -59

Flag	Coverage Δ
#unittests	45.78% <19.23%> (-28.02%)	:arrow_down:

Impacted Files	Coverage Δ
bids/analysis/analysis.py	29.57% <0%> (-59.21%)	:arrow_down:
bids/variables/io.py	42.71% <21.42%> (-32.42%)	:arrow_down:
bids/variables/entities.py	73.33% <66.66%> (-14.31%)	:arrow_down:
bids/analysis/transformations/base.py	17.87% <0%> (-68.72%)	:arrow_down:
bids/analysis/auto_model.py	26.15% <0%> (-61.54%)	:arrow_down:
bids/variables/kollekshuns.py	32.85% <0%> (-50.72%)	:arrow_down:
bids/analysis/transformations/munge.py	44.44% <0%> (-46.79%)	:arrow_down:
bids/variables/variables.py	42.22% <0%> (-46.23%)	:arrow_down:
bids/analysis/transformations/compute.py	45.71% <0%> (-40.96%)	:arrow_down:
... and 4 more

---

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 13c857f...90b861d. Read the comment docs.

[codecov[bot]]

Codecov Report

Merging #376 into master will decrease coverage by 0.31%. The diff coverage is 47.88%.

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
- Coverage   62.29%   61.98%   -0.32%     
==========================================
  Files          27       27              
  Lines        4554     4611      +57     
  Branches     1173     1189      +16     
==========================================
+ Hits         2837     2858      +21     
- Misses       1433     1462      +29     
- Partials      284      291       +7

Flag	Coverage Δ
#unittests	61.98% <47.88%> (-0.32%)	:arrow_down:

Impacted Files	Coverage Δ
bids/variables/kollekshuns.py	83.57% <100%> (ø)	:arrow_up:
bids/variables/entities.py	87.77% <100%> (+0.13%)	:arrow_up:
bids/variables/variables.py	83.54% <36.36%> (-4.85%)	:arrow_down:
bids/variables/io.py	72.24% <37.5%> (-3.01%)	:arrow_down:
bids/analysis/analysis.py	86.91% <50%> (-1.87%)	:arrow_down:
bids/analysis/transformations/compute.py	82.25% <57.89%> (-4.41%)	:arrow_down:

---

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 24ef1c8...2d32955. Read the comment docs.

[effigies]

@satra @tyarkoni I think this is ready for review. There's almost certainly a better way to do the integration, but this should work.

In passing, I had to fix the thing where run_info sometimes was the string "events" instead of a list of RunInfo objects. I'll split that out on Monday, and address it to the right issues.

[tyarkoni]

Hey, unrelated to the above thread, doesn't this implementation run counter to the suggestion previously that we handle as much of the densification logic as possible in the (implicit) to_dense call, housed in the Variable classes? I guess I'm not seeing why this stuff needs to go in Convolve. If we put it here, we will then need to put very similar-looking code in every other transformation that needs to implicitly convert from sparse to dense (accounting for the potential use of sparse acquisition). Is the thought that this is just temporary, and it'll eventually be refactored into the Variable hierarchy? If so, I'm fine with it. Just checking to see if that's what you're thinking.

[effigies]

I'm trying to think through this. Variables have no notion of a "default" sampling rate, as currently written, so moving this logic into to_dense() really only works if we have a global default and the condition that to_dense(None) (or similar) produces intelligent behavior. This doesn't seem to work nicely with the idea of a collection-level default sampling rate that may or may not be changed by a user...

Just to make clear, my distinction between typical and power users is pretty much whether they're doing this in JSON or Python. For the latter, most things are possible, so getting to the right behaviors for model writers is my priority.

[tyarkoni]

It doesn't necessarily have to live in to_dense, but it does need to abstracted out of Convolve, because there are other transformations that will need to deal with the same issue (e.g., if you try to orthogonalize a sparse variable with respect to a dense one, the sparse will be converted via to_dense. Actually, the way I'd intended this to work is that all conversion between variables is done before handing them to the Transformation-specific _transform logic.

If you look at the logic in the base Transformer class, which is admittedly kind of ugly at the moment, there's a place where _densify_variables is called. At this point, there's always an associated BIDSVariableCollection, so that's where the logic to determine the sampling rate should be going. The one (minor) hitch here is that there are currently no other Transformations that require dense variables. Currently, if you pass a dense=True argument to any transformation (undocumented, because not in spec), it will force any variables stored in the _densify class attribute to dense format before passing them to _transform. But in this case, Convolve must receive a dense variable, and there's no way to ensure that. The path of least resistance is probably to add a _force_dense attribute to the base Transformation class that mandates that input variables be densified before passing. Then that would route the inputs through _densify_variable, where the sampling rate would be determined via your existing logic, and then we just call to_dense() as currently implemented.

As far as as I can tell, this would elegantly solve the problem while adhering to the general principle that the transformation-specific logic should be kept as simple as possible, and the complexity should be hidden in the base class as much as possible (to make it easier to implement new transformations and minimize the maintenance burden). What do you think?

[effigies]

I think that seems like a decent approach. So a question: Should I clean up the tests here and get this merged, or start on that refactor? I think for @satra's purposes, it might be best to get a working sparse implementation in ASAP.

[tyarkoni]

Merging this as-is seems fine. The refactor will probably be pretty straightforward, and if any problems arise, they'll likely be coming from integration with 0.8 (though I think it should be fine), so we may as well hold off until that's ready.

[effigies]

Okay. I think I fixed up the failing test. @satra @mgxd Are you good with this? Have you had a chance to patch this into FitLins to test your stuff at all?

[satra]

@effigies, @mgxd - let's try that (testing this branch) with FITLNS before merging.

mathias: it could be a basic speech vs baseline model.

[effigies]

@mgxd Have you had a chance to give this a shot? Anything I can do to help?

[effigies]

I've merged the 0.8 upgrade in, so any work on this going forward should be done in the context of https://github.com/poldracklab/fitlins/commit/7ee02a2c9280171efffa619e1472c8d0e8dacf4a. @mgxd @satra Ping me for anything I can do to help.

[tyarkoni]

@effigies given that this will need to be updated to reflect #411, I wonder if this is also a good time to move the sr computation logic into to_dense() in SparseRunVariable. I think that will probably also make your life easier refactoring Convolve.

[effigies]

Okay. Once that's in, I'll try to re-assess and see if I can see the path forward your way. (I can't remember if it ever clicked, so I guess it didn't click very well...)

[tyarkoni]

The general idea was that computing the sr to take TA into account is going to be necessary any time densification occurs, not just in Convolve. So I think the only thing Convolve should be doing is checking to see if sparse acquisition was used, and if so, triggering to_dense.

Maybe it makes sense to add a helper method in SparseRunVariable that does all of that internally—i.e., something like a more sensibly named densifyIfSparseAcquisition().

Assuming the densification is done implicitly, I think you then won't need to do much else in Convolve; @adelavega's code should just treat the densified variable like any other DenseRunVariable, and compute the oversampling rate as usual. But I might be overlooking something.

[adelavega]

Assuming the densification is done implicitly, I think you then won't need to do much else in Convolve; @adelavega's code should just treat the densified variable like any other DenseRunVariable, and compute the oversampling rate as usual.

I just realized that I hadn't tested with Dense variables yet, and it will probably fail because I'm getting the onsets from var, not df. I'll change that in a sec.

[tyarkoni]

Yet another good reason to prioritize #320 (not that I needed more)! Will try to get to that shortly.

[adelavega]

Yep, I am adding at least a few tests for convolve as part of #411