Skip to content

Allow for SortingAnalyzer or BaseSorter in plot_* #3941

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

chrishalcrow
Copy link
Member

Following on from discussion in #3937, this PR allows the user to pass a SortingAnalyzer to plot_isi_distribution.

Will also add to plot_rasters, plot_unit_presence and plot_probe_map. Before I do, is the strategy here fine? I've replace the sorting argument with sorting_analyzer_or_sorting. I think most users use this as

si.plot_isi_distribution(analyzer)

so won't notice any difference.

For anyone passing sorting=sorting, I've deprecated the argument.

@chrishalcrow chrishalcrow added enhancement New feature or request widgets Related to widgets module labels May 21, 2025
@chrishalcrow chrishalcrow marked this pull request as draft May 21, 2025 09:34
@chrishalcrow chrishalcrow requested a review from alejoe91 May 21, 2025 09:34
@chrishalcrow chrishalcrow changed the title Add sa_or_sort to ISIDistributionWidget Allow for SortingAnalyzer or BaseSorter in plot_* May 21, 2025
@zm711
Copy link
Member

zm711 commented May 22, 2025

I think this is good from my perspective and should be extended to all widget functions. Maybe we should add a developer comment to mention which functions need the sorting and which need the analyzer, but that might also be clear by us adding the ensure_sorting or ensure_sorting_analyzer functions.

@samuelgarcia
Copy link
Member

Super merci Chris.

@chrishalcrow
Copy link
Member Author

Actually, I've made a bit of a mess of this.

The three functions, that this type of code applies to, now do three different things. I think this logic appears in a few places around the codebase, so I'd like to formally pick a solution. Let's vote!

Option 1 (UnitPresenceWidget. simple; less explicit):

def __init__(
    self, 
    sorting: BaseSorting | SortingAnalyzer
):
    sorting = self.ensure_sorting(sorting)

Option 2 (ISIDistributionWidget. Pretty simple. Quite explicit):

def __init__(
    self, 
    sorting_analyzer_or_sorting: BaseSorting | SortingAnalyzer
):
    sorting = self.ensure_sorting(sorting_analyzer_or_sorting)

Option 3 (RasterWidget. More complex, allows stricter typing, more obvious?)

def __init__(
    self,
    sorting: BaseSorter | None = None,
    sorting_analyzer: SortingAnalyzer | None = None,
):
    if sorting is None and sorting_analyzer is None:
        raise Exception("Must supply either a sorting or a sorting_analyzer")
    elif sorting is not None and sorting_analyzer is not None:
        raise Exception("Should supply either a sorting or a sorting_analyzer, not both")
    elif sorting_analyzer is not None:
        sorting = sorting_analyzer.sorting

    # could skip this, but it allows for backwards compat with WaveformExtractor too!
    sorting = self.ensure_sorting(sorting)

I vote Option 2. Please vote @zm711 @samuelgarcia @alejoe91 @h-mayorquin

@alejoe91
Copy link
Member

alejoe91 commented Jun 3, 2025

Option 2 +1

@zm711
Copy link
Member

zm711 commented Jun 3, 2025

I would vote for 2.

Option 2 +1

when would you want 1 @alejoe91 ?

@chrishalcrow
Copy link
Member Author

I think Alessio was saying "add one vote to option 2". So that's three votes, and I've implemented it!

@chrishalcrow chrishalcrow marked this pull request as ready for review June 3, 2025 12:35
@alejoe91
Copy link
Member

alejoe91 commented Jun 3, 2025

I think Alessio was saying "add one vote to option 2". So that's three votes, and I've implemented it!

Yes it was a boomer emoji attempt ahhaha

@zm711
Copy link
Member

zm711 commented Jun 3, 2025

My bad. I can see what you mean now. haha.

@samuelgarcia
Copy link
Member

I vote for 2.

@h-mayorquin
Copy link
Collaborator

2 is OK for me.

I think this is one of those cases where a positional only might make sense? not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request widgets Related to widgets module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants