Remove Non-Serializable writing of json #3482
Conversation
zm711
added
sorters
for more information, see https://pre-commit.ci
| raise RuntimeError( | ||
| "This recording is not serializable and so can not be sorted. Consider `recording.save()` to save a " | ||
| "compatible binary file." | ||
| ) |
There was a problem hiding this comment.
I think we should keep the possibility of running a sorting on a recording living only in memory.
And so no dump into a file.
This could be usefull for benchmark of simulated in memeory.
So I would pass silently no ?
There was a problem hiding this comment.
This was inspired by the issue I linked where we asked you to comment. fine to let it pass if you're okay with it, but do you want to read that issue and comment there or here on your opinions about it.
There was a problem hiding this comment.
Based on your comment in the issue what I would argue is we add this runtime error but with a message like:
This recording is not serializable and so can not be sorted. Consider recording.save() to generate a compatible binary file. We plan to support in memory sorting for some sorters in the future
But it is not currently really supported. So we should truly warn people about something impossible and then you remove the error when you have the sorting in-memory actually worked on.
There was a problem hiding this comment.
Sam and I discussed this and he is okay with the error for now. Any opinions on adding either
- a comment saying we should support in-memory in the future
- saying in the error message that we plan to support in the future
There was a problem hiding this comment.
No I think it's fine to as is :)
The note says remove after 0.100 and this caused an issue recently #3448 with the error not being super clear (because recording was None) the error the user saw was that None didn't have get_num_channels). Since this isn't supported we shouldn't write this here, right? I have run local tests because I need to run, so will update after the CI if needed.