spikeinterface icon indicating copy to clipboard operation
spikeinterface copied to clipboard

`.save_to_folder()` fails for the `InjectDriftingTemplatesRecording`

Open cwindolf opened this issue 1 year ago • 12 comments

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!

cwindolf avatar Jul 02 '24 16:07 cwindolf

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

alejoe91 avatar Jul 02 '24 16:07 alejoe91

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?

cwindolf avatar Jul 02 '24 16:07 cwindolf

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

self._serializability["json"] = False

alejoe91 avatar Jul 02 '24 17:07 alejoe91

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

https://github.com/SpikeInterface/spikeinterface/blob/ed9d1b5c7de0e3b0af351cbe1454ce74a4fec54c/src/spikeinterface/core/template.py#L250-L261

Could you share a minimal example where this fails?

h-mayorquin avatar Jul 02 '24 18:07 h-mayorquin

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

cwindolf avatar Jul 02 '24 18:07 cwindolf

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

alejoe91 avatar Jul 02 '24 18:07 alejoe91

@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!

alejoe91 avatar Jul 02 '24 18:07 alejoe91

OK. Let's discuss it tomorrow.

h-mayorquin avatar Jul 02 '24 18:07 h-mayorquin

@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)

alejoe91 avatar Jul 03 '24 09:07 alejoe91

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

cwindolf avatar Jul 03 '24 14:07 cwindolf

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

rec1.get_parent().drifting_templates.templates_array_moved.shape

alejoe91 avatar Jul 03 '24 15:07 alejoe91

Thanks, good to know!

cwindolf avatar Jul 03 '24 15:07 cwindolf