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

.save_to_folder() fails for the InjectDriftingTemplatesRecording #3125

Open
cwindolf opened this issue Jul 2, 2024 · 12 comments · May be fixed by #3130
Open

.save_to_folder() fails for the InjectDriftingTemplatesRecording #3125

cwindolf opened this issue Jul 2, 2024 · 12 comments · May be fixed by #3130
Labels
bug Something isn't working hybrid Related to Hybrid testing

Comments

@cwindolf
Copy link
Collaborator

cwindolf commented Jul 2, 2024

When trying to save a hybrid recording, I get the error TypeError: Object of type DriftingTemplates is not JSON serializable. This makes sense, because DriftingTemplates inherits from Templates which is a dataclass and not a BaseExtractor, so it will not be .to_dict()ed by https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/core/base.py#L423 even though it has a .to_dict() method.

I'm not sure how to resolve this? One could maybe make Templates subclass BaseExtractor while still being a dataclass? Although it does feel a bit strange to json-serialize a large array of templates. Anyway, wanted to see what you all think, thanks!

@alejoe91
Copy link
Member

alejoe91 commented Jul 2, 2024

Thanks! One simple option is to make the InjectDriftingTemplatesRecording not JSON serializable, so it will be saved to pkl at all instances

@alejoe91 alejoe91 added bug Something isn't working hybrid Related to Hybrid testing labels Jul 2, 2024
@cwindolf
Copy link
Collaborator Author

cwindolf commented Jul 2, 2024

Oh! That sounds good. Happy to make a PR for that -- is it done by implementing check_if_json_serializable(self)? Or would Templates need to inherit from BaseExtractor?

@alejoe91
Copy link
Member

alejoe91 commented Jul 2, 2024

@cwindolf you can just add this at the end of the init:

self._serializability["json"] = False

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Jul 2, 2024

Mmm the Templates should be json serializable, it has a dict:

def to_dict(self):
return {
"templates_array": self.templates_array,
"sparsity_mask": None if self.sparsity_mask is None else self.sparsity_mask,
"channel_ids": self.channel_ids,
"unit_ids": self.unit_ids,
"sampling_frequency": self.sampling_frequency,
"nbefore": self.nbefore,
"is_scaled": self.is_scaled,
"probe": self.probe.to_dict() if self.probe is not None else None,
}

Could you share a minimal example where this fails?

@cwindolf
Copy link
Collaborator Author

cwindolf commented Jul 2, 2024

@h-mayorquin it does have a to_dict() method, but it doesn't inherit from BaseExtractor, so it won't go through the usual code path (see the line I link to in the top post). Sorry, my example is a bit of a pain to share, but I think any example should work. Note that one needs to make sure the sorting used is a dumpable one, else provenance.json just has an error message and you won't get to the point of trying to serialize the templates.

I think for the same reason @alejoe91 just setting self._serializability["json"] is not enough, because Templates is not a BaseExtractor so it won't know how to reconstruct itself -- the code path that's hit when I then later try to load up the full hybrid recording object via sc.load_extractor(my_hybrid_recording_dir / "provenance.json") then goes throughBaseExtractor.load() stuff. I think Templates needs to subclass BaseExtractor, but I don't know enough about how these things work yet...

@alejoe91
Copy link
Member

alejoe91 commented Jul 2, 2024

@h-mayorquin maybe we just need to extend the SIJsonEncoder to support Templates?

@alejoe91
Copy link
Member

alejoe91 commented Jul 2, 2024

@cwindolf no we don't need to inherit from BaseExtractor, because that's for a Recording or Sorting object.

@h-mayorquin let's discuss about this tomorrow!

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Jul 2, 2024

OK. Let's discuss it tomorrow.

@alejoe91
Copy link
Member

alejoe91 commented Jul 3, 2024

@cwindolf can you test this one #3130? With this PR the provenance is saved to pkl if the recording is not JSON serializable (and this is the case for then InjectDriftingTemplatesRecording)

@cwindolf
Copy link
Collaborator Author

cwindolf commented Jul 3, 2024

Thanks @alejoe91 ! This is great. rec.check_serializability(type='json') fails now and there is a provenance.pkl saved. I can access the .drifting_templates.templates_array_moved property and everything. I did this on a .frame_slice() for testing like so:

rec.frame_slice(0, 1000).save_to_folder(scratch_dir)
rec1 = si.load_extractor(scratch_dir / "provenance.pkl")
rec1._kwargs["parent_recording"].drifting_templates.templates_array_moved.shape

@alejoe91
Copy link
Member

alejoe91 commented Jul 3, 2024

@cwindolf you can also do this! (better API)

rec1.get_parent().drifting_templates.templates_array_moved.shape

@cwindolf
Copy link
Collaborator Author

cwindolf commented Jul 3, 2024

Thanks, good to know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hybrid Related to Hybrid testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants