search

cvnlab / GLMsingle

A toolbox for accurate single-trial estimates in fMRI time-series data
BSD 3-Clause "New" or "Revised" License
101 stars 45 forks source link

minor ports from matlab to python (re: cases with no repeats; new FIR summary voxel viz) #124

Closed jacob-prince closed 10 months ago

jacob-prince commented 10 months ago

these two new commits contain ports of the following commits of matlab code made by @kendrickkay

371754299834852bc31eae126f63cee97441d068: making sure dmetric is nan if no repeats 881b5a31c917462027e30e210c0042c718969db5: adding warnings if no repeats 31f9eaa46e6f8c01cde49315826a0aef5cc674f1: runwise_FIR summary voxel visualization 3c0c435ca3bf34e3e37ef6d3c6229d263a881ca9: adding sanity check to prevent conds occurring at same time.

for some reason there is weirdness with whitespace in the commit of glmsingle.py; I will try to avoid this in future commits. would recommend using "ignore whitespace" to review the code.

kendrickkay commented 10 months ago

Oh, I see the "ignore whitespace" button. Cool.

kendrickkay commented 10 months ago

I wouldn't worry about trying to avoid the whitespace stuff in the future since the little button makes it all fine.

jacob-prince commented 10 months ago

Yeah, I should have just made sure to "discard all changes" before editing any part of the code. Now I know. That will prevent whitespace issues moving forward.

iancharest commented 10 months ago

Do we know if this still works with our tests?

jacob-prince commented 10 months ago

I am working on developing proper suites of unit test cases as we have discussed. In the meantime, I am testing these ports primarily using example01 dataset. For each commit, I modify the data to test whatever feature is being implemented. For example by messing with the design matrix to trigger the error associated with having two conditions at the same onset time. If you'd prefer, we can hold off on merging until I have more fully expanded the unit tests. And/or, please suggests additional tests I should run at this time...

kendrickkay commented 10 months ago

I think you can/should go ahead and do the merge. The fuller test suite stuff can come later.

Jacob and Ian, if you recall, we do have a "manual" test suite that I had run in the past on 10+ (small-scale) datasets. Jacob, when you are ready, we can go over how that is set up if you have forgotten. Perhaps the goal is to get those tests fully automated.

jacob-prince commented 10 months ago

I have added a few commits that handle all the extra_regressors functionality. let me know if things look good and I will merge.