search

flatironinstitute / CaImAn-MATLAB

Complete Matlab pipeline for large scale calcium imaging data analysis
GNU General Public License v2.0
252 stars 147 forks source link

Errors while handling loaded workspace variables as input #59

Closed HagaiHargil closed 7 years ago

HagaiHargil commented 7 years ago

I stumbled upon a bug when trying to run run_pipeline.m on variables that were already loaded into the memory.

It revolves around the handling the name variable in the for loop starting at line 19: When the variable is already loaded in memory, name = files(i).name; is a full 3D matrix ("tensor"). Thus, when in line 24 you write: 'h5_filename',[name(1:end-4),'_nr.h5'], it actually takes the values of the matrix as the filename.

We solved it on our end by keeping name a 3D matrix, and adding a .filename field for the files struct. Thus, lines 24, 26, 29, and 31 have been changed to look something like [files(i).filename,'_nr.h5'] (that's line 24, for example).

The reason this is not a pull request is that we're actually using a different module to create the files variable, which is called uipickfiles. It's much easier to work with, and allows you to choose specific files from specific folders without any issues. It requires changing a couple of other lines throughout run_pipeline.m and normcorre_batch.m, though, and so it's not a full drop-in replacement as of yet. If we'll integrate it completely into our code and you'll be interested in having it, I'd gladly submit a full pull request for it.

epnev commented 7 years ago

Thanks for the heads up @HagaiHargil I hadn't encountered this bug so far. Do you think that would also be resolved by leaving the files struct as is, but change the name variable to filename, i.e., filename = files(i).name?

Thanks also for suggesting uipickfiles. I'll look into it.

HagaiHargil commented 7 years ago

I think your suggestion will help, but won't resolve this issue completely. The name variable in lines 24 and 29 can either hold a pointer to a .tif file (for example) or hold the actual data (i.e. it would be a 3D matrix). The same name variable in lines 26 and 31 should be just the filename and in case.

So your proposed filename variable should come in place of lines 26 and 31, but I'd change the name variable of lines 24 and 29 to data, to signify its different use cases and contents. It's a bit of a "patchy" solution, since you're currently acquiring this variable from the files struct, but it's a good first step.

epnev commented 7 years ago

I made some changes in how the filename is specified and changed when calling normcorre. From my understanding, the problem you mentioned only arises if you have multiple files already loaded in memory which is not what the file assumes (although it can appear into a pipeline). Feel free to reopen this issue if you think that there are still issues.