Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem with using reloaded cnmf object #1305

Merged
merged 2 commits into from
Mar 31, 2024
Merged

Conversation

EricThomson
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)

Tests

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

@pgunn
Copy link
Member

pgunn commented Mar 22, 2024

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
Copy link
Contributor Author

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 EricThomson marked this pull request as ready for review March 30, 2024 16:14
@EricThomson
Copy link
Contributor Author

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. 😬

@EricThomson EricThomson merged commit 0452184 into dev Mar 31, 2024
3 checks passed
@pgunn pgunn deleted the fix_reload_typing branch May 3, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants