Hybrid recordings

[DradeAW]

[DradeAW]

I haven't tested the code yet. @samuelgarcia is this the architecture we were talking about yesterday for AddTemplatesRecording ?

[DradeAW]

I believe that the only things left are tests and optimization.

[samuelgarcia]

Hi Aurelien, I will have a deep look and will make final feedback soon.

[yger]

I can also have a look, and if you are planning to discuss that more in depth, ready to join

[samuelgarcia]

Hi AUrelien, here some feedback:

1. ould you add some test files.
2. AddTemplatesRecording could be in the core I think.
3. AddTemplatesRecording not super happy with the name (@alejoe91 ?)
4. HybridUnitsRecording/HybridSpikesRecording could be the same class with options. It will more simple for the end user.

I also made some comments on the fly.

@alejoe91 : there are some question for you about naming and module organisation. If you have time to comment, this would be cool.

[DradeAW]

I don't know about making the HybridUnitsRecording and HybridSpikesRecording the same class. To me it seems that they have very different parameters, and thus would make it harder for the user to understand how to use it.

Do you have an idea to make this work?

[alejoe91]

I don't know about making the HybridUnitsRecording and HybridSpikesRecording the same class. To me it seems that they have very different parameters, and thus would make it harder for the user to understand how to use it.

Do you have an idea to make this work?

I agree with @Dradeliomecus on this. I think two classes are better because in terms of implementation they would also do different things and require different inputs!

[DradeAW]

@samuelgarcia In the last commit, I added a bit of messy code in BaseSorting. I don't know how you want to format it?

[DradeAW]

I believe that everything is done!!! (except for the is_dumpable and _kwargs thing)

[DradeAW]

@alejoe91 I've made the changes we talked about, but the traces for HybridUnitsRecording.save(n_jobs>1) seem to be differ from the original recording for some reason. Any idea what's happening?

By the way, the workflow for "full tests" seem to not test "comparison" @samuelgarcia

[alejoe91]

@alejoe91 I've made the changes we talked about, but the traces for HybridUnitsRecording.save(n_jobs>1) seem to be differ from the original recording for some reason. Any idea what's happening?

By the way, the workflow for "full tests" seem to not test "comparison" @samuelgarcia

It tests a module only if some detected changes are there..but you're right!!! We are missing comparison!!

Do you mind if I push directly here to add it?

[DradeAW]

Do you mind if I push directly here to add it?

Not at all, go for it!

[alejoe91]

@DradeAW I fixed tests here: https://github.com/SpikeInterface/spikeinterface/pull/1014

We can merge that small change (I improved the check_json) and then test should be triggered here

[DradeAW]

Ok I implemented everything we talked about.

Now the only thing left (I believe) is to figure out why the tests are failing.

[alejoe91]

Now tests are triggered!

[alejoe91]

@samuelgarcia this is good to go from my side

@DradeAW can you test it one more time to triple check that everything works (and hybrid recordings look good)?

[DradeAW]

Other than the review I left, everything looks good on my end!

[alejoe91]

@samuelgarcia this is good to go on our side!

[samuelgarcia]

Hi Aurelien, I read it again. It is perfect. Thanks a lot for this big stuff.

I will be very annoying, I know. I have an ultimated concern. I do not like the function/class name add_templates() / AddTemplatesRecording(). It is really confusing if you think about it. Maybe inject_template() would be more clear for the end user. Maybe you have other name idea.

I can do it if you want and if we agree a good name.

[DradeAW]

Thanks :)

Yeah I'm really bad at finding names, so I will let you decide on what the naming should be (I usually like the names you pick).

[samuelgarcia]

FInally Alessio did not totally with me so I did the name change. I am sorry. Ready to merge on my side.