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

Implement a simple system to have backward compatibility for Analyzer extension #3215

Merged
merged 23 commits into from
Jul 18, 2024

Conversation

samuelgarcia
Copy link
Member

@samuelgarcia samuelgarcia commented Jul 16, 2024

This simple system enabling to hack on load self.params and self.data when a new features are implemented in an analyzer extension that imply new parameters and new variable in data.

It was for instace the case for template similarity

@samuelgarcia samuelgarcia added this to the 0.101.0 milestone Jul 16, 2024
Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Do we not need to set need_backward_compatibility_on_load for the similarity?

It seemed to work without that though it gave a different error.

@samuelgarcia
Copy link
Member Author

oups

@zm711
Copy link
Collaborator

zm711 commented Jul 16, 2024

Here's what I don't understand. Now that we've added the need_backward_compatibility_on_load now it is going back to failing with the first error where it can't find the max_lag_ms. I need to read this a bit better why it worked when that line was missing, but isn't working now.

@zm711
Copy link
Collaborator

zm711 commented Jul 16, 2024

Since the most recent commit, nothing I do works. I tried going back in and just deleting need_backward so it was like your original commit. But now regardless of what I do it gives me the missing max_lag_ms error.

@alejoe91 alejoe91 added the core Changes to core module label Jul 17, 2024
@zm711
Copy link
Collaborator

zm711 commented Jul 17, 2024

Here's the current bug in merge units


  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\sortinganalyzer.py:939 in merge_units
    if len(units) < 2:

TypeError: object of type 'int' has no len()

@zm711
Copy link
Collaborator

zm711 commented Jul 17, 2024

I have to finish up a presentation at the moment, so I can test another round in the background, but I can't track down this bug. Just let me know when ready for another test :)

@zm711
Copy link
Collaborator

zm711 commented Jul 17, 2024

Never mind I put the list in as a list instead of list of list. Let me try again :)

@zm711
Copy link
Collaborator

zm711 commented Jul 17, 2024

Okay the merge code works from loading an analyzer from 08MAY24 :) !!

@alejoe91
Copy link
Member

Okay the merge code works from loading an analyzer from 08MAY24 :) !!

For me it works for waveform extractor folders loaded as analyzers from last year :P

@samuelgarcia
Copy link
Member Author

I hope you both have tried sigui for this dataset.

@zm711
Copy link
Collaborator

zm711 commented Jul 17, 2024

It has a probegroup. Has that been patched yet?

@yger
Copy link
Collaborator

yger commented Jul 17, 2024

Not sure the problem is fully solved. While I can load the analyzer you gave me @alejoe91 for testing the merging, when applying new functions recursively, I still have errors such as

nbefore = int(self.params["ms_before"] * self.sorting_analyzer.sampling_frequency
return nbefore

KeyError: 'ms_before'

@alejoe91
Copy link
Member

@yger I know, will push a fix tomorrow!

@alejoe91
Copy link
Member

@yger fixed! The templates were not an extensions so they required a fix before in the code

Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Just one tiny error comment.

@alejoe91 alejoe91 merged commit 63b295c into SpikeInterface:main Jul 18, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants