zivlab / CellReg

Registration of cells across sessions.
GNU General Public License v2.0
70 stars 33 forks source link

some code improvement required and high resource usage in a short time. #17

Closed zhaolun7 closed 2 years ago

zhaolun7 commented 2 years ago

nansum is not availble in matlab 2022a, use sum(xx , 'omitnan') to replace two places. image

When I run with sample, I observe a high resource usage: image

and in step 2, if I choose 'translation' it will fail: image image

Sheintuch commented 2 years ago

Hi. Thank you for noticing these errors and opening this issue.

  1. I changed the nansum to sum('omitnan') so that CellReg will work for matlab 2022a (see new CellReg version v1.5.3).
  2. I fixed the error of unrecognized variable 'unrotated_spatial_footprints' and now you can choose 'translations' in the alignment type without running into an error.
  3. Regarding the high resource usage - is that a problem that causes an error? Does it interfere with working on other things at the same time with your computer? This would probably require a significant change in the code in order to fix. Did you have a specific suggestion in mind?
zhaolun7 commented 2 years ago

@Sheintuch Thank you for the fix. My computer have 32g memory so this procedure works well. But it may be unfriendly to other lower memory pc. I haven't looked closely at the implementation details, but I'm surprised that running through the sample data uses so much memory. Thanks again for your fixing other problems.

zhaolun7 commented 2 years ago

image if I choose 'translate' only, it works well in step 2, but next step(step 3) meet error.........

Sheintuch commented 2 years ago

I am running the script with only translations and can go over all of the steps without encountering any error. Are you sure you correctly updated to the new version? Did you use the git pull command or did you download the repository again? If you downloaded the repository, did you make sure you delete the older version from your computer?

zhaolun7 commented 2 years ago

In step 3, it do ouput the result. but I see a error message in control concole. it may means something wrong.. image I delete the old code, download a new zip file from github. then run CellReg_setup.m image and I compare the difference. It's same with the version 1.5.3 image

Sheintuch commented 2 years ago

From the error message you posted here it seems that it is happening in step 2 when trying to save the results (otherwise you would see in the command window that it finished with aligning the sessions). Can you see if this is correct and that indeed you see this error in the command window even without starting step 3?

I am not sure why this happening as I am not getting this error. Just to make sure, the only thing you are changing from the default parameters is the choice of 'translations' in the alignment type? And other than that, you simply load new data in step 1, set the microns per pixel and then press the align images button?

zhaolun7 commented 2 years ago

In step 2 it may open something like a thread pool to accelerate calculation(I am new to matlab). I don't notice that, and start step 3 before step 2 is finished. now it works ok. Thank you for the information.

zhaolun7 commented 2 years ago

I think the command parfor is the root of short high memory resource requirement. I think matlab will try to require resources as many as a pc can offer to save time if we don't limit something. It will cause pressure in a laptop. If we can offer a option for users to limit the max thread working at the same time, it will be better. Just an advice...

Sheintuch commented 1 year ago

Hi. Thank you for the advice. There is an option for users not to use the parallel processing if you are running CellReg through the 'demo' script instead of the GUI. You will need to change the 'use_parallel_processing' variable in line 75 in the 'demo' script from true to false. Let me know if this solves the high memory requirement issue for you and if you need it also for the GUI. If you do need it specifically for the GUI, I will work on adding this feature as a checkbox.