flatironinstitute / CaImAn

Computational toolbox for large scale Calcium Imaging Analysis, including movie handling, motion correction, source extraction, spike deconvolution and result visualization.
https://caiman.readthedocs.io
GNU General Public License v2.0
640 stars 370 forks source link

Fix problem with using reloaded cnmf object #1305

Closed EricThomson closed 8 months ago

EricThomson commented 8 months ago

Description

Issue raised with using reloaded data, the way things are serialized with hdf5, they are loaded as byte-encoded, so when you try to change parameters things don't work (e.g., filename validation throws an error). Rather than digging deeply into serialization in recursively_load_dict_contents_from_group() (or the corresponding save function), I added a check for the dtype in check_consistency() and if the filenames are not string, do the conversion. This is a bit hacky, but I tested it and it works 😬

Maybe later we can do more internal validation in the load function, but for now I think this is a good workaround.

Thanks @JohnStout for pointing it out.

Closes #1264

Type of change

Tests

caimanmanager test works fine. I tested saving/loading and then changing parameters which was broken before, and now it works.

pgunn commented 8 months ago

This will only handle if it's bytes, but given that that's the most likely cause of this and there's no entirely general way to handle whatever else might be handled by check_consistency() it's a good solution for now.

We could in theory handle other things, like pathlib objects, but a lot would need to change if we ever wanted to support those.

EricThomson commented 8 months ago

This will only handle if it's bytes, but given that that's the most likely cause of this and there's no entirely general way to handle whatever else might be handled by check_consistency() it's a good solution for now.

We could in theory handle other things, like pathlib objects, but a lot would need to change if we ever wanted to support those.

It's definitely not perfect. This handles the current failure point (how filenames are handled by the hdf5 encoder/reader), but won't handle every possible problem. I'd tentatively place this in the "good enough for now" shed.

EricThomson commented 8 months ago

I wasn't happy with checking for not string so I added explicit check for byte-encoding instead. Also, I had deleted the previous check for string as filename instead of list, so added that back in. 😬