shalinshah1993/temporalDNAbarcodes

JOSS Reviewer comments: Demo not working

Closed this issue · 8 comments

lbugnon commented

Ref openjournals/joss-reviews#2026
Hi, I'm trying to run the demo but it goes with the following error:

Error using lif2mat (line 35)
The size of the right hand side did not match the size of the indexing on the left hand side.

It appears that target frame_size is larger than framebycell.

shalinshah1993 commented

What command did you type and what file did you run it on?

lbugnon commented

Sorry, I see that the avi file was deleted. Now I've tried sample_data.lif and it's working

  • 👍1
lbugnon commented

Now lif is working, but findalllocalizations throws that 'filename' not in the path. Maybe I'm missing something (already addded this repo to the path)

Error using VideoReader/init (line 601)
The filename specified was not found in the MATLAB path.

Error in VideoReader (line 171)
obj.init(fileName);

Error in findalllocalizations (line 19)
videoObj = VideoReader(strcat('video/', file));

shalinshah1993 commented
lbugnon commented

I'm trying to follow the readme pipeline. After lif2mat,

findalllocalizations('sample_data.mat')

I tried moving the file from tmp/mat to /mat but still same error

EDIT: now I see findalllocalizations is expecting a .avi, in the video/ folder, still not working with readme insttructions, please check the readme, I'm probably missing something:

findalllocalizations('sample_data.avi')
Not enough input arguments.

Error in findalllocalizations (line 30)
cur_frame(cur_frame < threshold * mean(cur_frame)) = 0;

shalinshah1993 commented

Thank you for pointing this issue out, it was an error in README, I fixed it. Basically, findalllocalizations expects a video file in the video folder. I also added a sample video so you don't need to do any moving between folders yourself.

please do a git pull and then try running:

findalllocalizations('sample_data.avi')

This should find all the spots in the video file sample_data.avi found in video folder.

lbugnon commented

Ok, some final issues to solve, mainly related with README and a proper testing scheme. Remember that having a manual or automatic testing procedure for the modules is a requisite in JOSS

  • findalllocalizations and other functions (as estdrift, crctdrift and calcdatatrend) expect other parameters besides filename. This should be stated in readme or set by default. Please check all the modules so one can arrive to an expected outcome.

  • When using findalllocalizations with threashold=3 and 5, the following error appeears at some frames of sample_data

Error using insertShape
Expected POSITION to be finite

It seems to work ok when plotting is disabled.

  • In the README, "findlocalizations"appears in place of "gettemporalbarcode".

  • Also the computer vision package is needed when toDisplayFrames is enabled.

  • Wavelet package is required for denoising in gettemporalbarcode.

  • There is some problem with R2018a when toDisplayFrames=1 (as you stated to use R2018b, this is not an issue, just saying)

  • 👍1
shalinshah1993 commented

Thank you for raising these points. @lbugnon

Please see below:

  • Thank you for pointing this issue. This has now been resolved with an updated README. Also, each file header has a short module description which states the expected parameter values.

  • Thank you for pointing this. It seems I am not able to see this using this command:
    findalllocalizations(<file_name>, 3, 1, 1)

    However, i do see it when I set a very high threshold:
    findalllocalizations('ideal data 128x128.avi', 5, 1, 1)

    This issue means that no point was detected in a frame. I've created a new issue and fixed it now. #6

  • Thank you for pointing this out. The function use case in README has been fixed.

  • Somehow MATLAB dependency library didn't state those as required. Thank you for letting us know. I have added both of them as required dependencies in the README (https://github.com/shalinshah1993/temporalDNAbarcodes#dependencies).

  • Thank you for pointing this, I've created an issue referring to this. A fix suggesting that a MATLAB upgrade to >= R2018b is required.