-
Notifications
You must be signed in to change notification settings - Fork 216
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
base: main
Are you sure you want to change the base?
Conversation
SortingAnalyzer
or BaseSorter
in plot_*
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 |
Super merci Chris. |
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 |
Option 2 +1 |
I would vote for 2.
when would you want 1 @alejoe91 ? |
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 |
My bad. I can see what you mean now. haha. |
I vote for 2. |
2 is OK for me. I think this is one of those cases where a positional only might make sense? not sure. |
Following on from discussion in #3937, this PR allows the user to pass a
SortingAnalyzer
toplot_isi_distribution
.Will also add to
plot_rasters
,plot_unit_presence
andplot_probe_map
. Before I do, is the strategy here fine? I've replace thesorting
argument withsorting_analyzer_or_sorting
. I think most users use this asso won't notice any difference.
For anyone passing
sorting=sorting
, I've deprecated the argument.