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.

Sign up for GitHub

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

Remove Non-Serializable writing of json #3482

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Remove Non-Serializable writing of json #3482

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
146d34a
remove writing text_file
zm711 Oct 15, 2024
5e832c9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 15, 2024
67f50a4
Merge branch 'main' into basesorter
zm711 Oct 22, 2024
0c5e8bb
Merge branch 'main' into basesorter
zm711 Oct 24, 2024
af1709a
Merge branch 'main' into basesorter
zm711 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/spikeinterface/sorters/basesorter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,10 @@ def initialize_folder(cls, recording, output_folder, verbose, remove_existing_fo
elif recording.check_serializability("pickle"):
recording.dump(output_folder / "spikeinterface_recording.pickle", relative_to=output_folder)
else:
# TODO: deprecate and finally remove this after 0.100
d = {"warning": "The recording is not serializable to json"}
rec_file.write_text(json.dumps(d, indent=4), encoding="utf8")
raise RuntimeError(
"This recording is not serializable and so can not be sorted. Consider `recording.save()` to save a "
"compatible binary file."
)
Comment on lines +148 to +151
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@zm711 zm711 Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sam and I discussed this and he is okay with the error for now. Any opinions on adding either

  1. a comment saying we should support in-memory in the future
  2. saying in the error message that we plan to support in the future

@alejoe91

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think it's fine to as is :)


return output_folder

Expand Down