ENH: Calculate default sampling rate for SparseRunVariable.to_dense

[effigies]

Proof-of-concept to accompany #568.

Related:

• Upscale to collection sampling rate prior to resampling #568 (comment)
• resample: ValueError: x and y arrays must be equal in length along interpolation axis. #361 (comment)

[adelavega]

Oh, nice. I was literally just about to comment "Great idea but do you have an implementation" ;)

[effigies]

Yeah, it seemed a bit harsh to push without making sure it worked.

@tyarkoni Any concerns with this?

[adelavega]

So I tried it with this simple example:

In [26]: onset                                                                  
Out[26]: [0, 2, 4, 6, 8, 10]

In [29]: duration                                                               
Out[29]: [1, 1, 1, 1, 1, 1]

The resulting sampling_rate was 1

With this example:

In [52]: onset = [0, 0.1, 0.2, 0.3, 0.4, 0.5]                                   

In [53]: duration = [0.1] * len(onset) 

I got sampling rate of 10hz.

That seems pretty reasonable to me. My main concern would be if you had very high frequency events, and you got an absurdly high sampling_rate that had performance issues. It might not really make a difference once you down sample anyways, but it'll cost you performance wise.

FWIW, I think the upsampling/downsampling that occurs in Convolve does something similar to this.

Otherwise, all other calls of to_dense should be unaffected since previously it was a required argument.

If we make this change, you'll want to update the spec.

[effigies]

If you have very high-frequency events, you presumably want to represent them at roughly that frequency...

I think the bigger danger is somebody who got a little too excited about precision timing in their fMRI design, and jitters by milliseconds, so the stimuli are all N seconds +/- 5ms, then you end up with a ~1KHz dense variable that isn't worth the precision. But for such a case, ToDense can be called with an explicit sampling rate.

[codecov]

Codecov Report

Merging #571 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #571      +/-   ##
========================================
+ Coverage   82.97%    83%   +0.02%     
========================================
  Files          23     23              
  Lines        2966   2971       +5     
  Branches      749    750       +1     
========================================
+ Hits         2461   2466       +5     
  Misses        323    323              
  Partials      182    182

Flag	Coverage Δ
#unittests	83% <100%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
bids/variables/variables.py	88.13% <100%> (+0.25%)	⬆️

---

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 a23fd41...295e73a. Read the comment docs.

[effigies]

If we make this change, you'll want to update the spec.

I don't know that this actually changes anything WRT the spec. Currently ToDense says that no specified SR means the software is free to do something "reasonable". And this doesn't change ToDense, as all previous calls to .to_dense() had to have pre-specified SRs.

[adelavega]

Nice. Well this certainly fits the bill of "something reasonable"

[effigies]

Okay. I'm going to go ahead and merge this, as it only turns things that would previously have been TypeErrors and does something with them. Tal can revert if he's outraged.