search

MouseLand / suite2p

cell detection in calcium imaging recordings
http://www.suite2p.org
GNU General Public License v3.0
334 stars 239 forks source link

checking h5py key to open binaries before data_path #1042

Closed arielleleon closed 7 months ago

arielleleon commented 9 months ago

Hello, In the default_ops, it states that if a h5py contains elements to process that data_path will not be checked. To fix this, I re-ordered the logic in the io.utils.find_files_open_binaries so that the h5py is checked first and bypasses the data_path search. This PR is linked to an issue I opened just now. Thanks!

https://github.com/MouseLand/suite2p/issues/1041

carsen-stringer commented 8 months ago

Hi @chriski777 could you please test this pull request and issue next time you have time?

Thanks,

Carsen

arielleleon commented 8 months ago

@carsen-stringer Please let me know if you have any questions about this PR. There is a separate issue here where I logged more information on the issue.

https://github.com/MouseLand/suite2p/issues/1041

Thanks for looking into this!

carsen-stringer commented 8 months ago

Is there any way to make this fix backwards compatible?

arielleleon commented 8 months ago

@carsen-stringer - I took another look at the code and the solution might be something different and I would like to get your thoughts as I'm still new to this library. The comments in the default_ops.py file on L22 states "take h5py as input (deactivates data_path)". But then here on L265 of the utils.py the code checks the data_path which was set here L458 of run_s2p.py. My assumption was that if you are checking a directory structure for h5's, you would not set the h5 key in the first place. Which is why I swapped the data_path check in the find_files_open_binaries in utils.py (my original solution). But maybe the solution is to not set the data_path in the first place here. Let me know what you think and I would be happy to update the PR.

arielleleon commented 8 months ago

Also - to answer your question via Kayvon - My solution does fix our problem but I haven't checked if it breaks how others are using it.

carsen-stringer commented 8 months ago

thanks for all the info, I think it makes sense what you did (it makes things more consistent across data types) -- but we'll need to update the tests. I'll get back to you after SfN with a solution. in the meantime please instruct people (like Kayvon :)) to install your fork

arielleleon commented 8 months ago

Thanks!

carsen-stringer commented 7 months ago

okay cool most of the tests are passing, I see one tiny bug to sort out with the previous way of running things, will fix tomorrow

arielleleon commented 7 months ago

Thank you @carsen-stringer