Skip to content

Add pydantic curation model, improve merging rules, and add splitting model #3760

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 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ dependencies = [
"neo>=0.14.0",
"probeinterface>=0.2.23",
"packaging",
"pydantic",
]

[build-system]
Expand Down
13 changes: 9 additions & 4 deletions src/spikeinterface/core/sorting_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,13 @@ def random_spikes_selection(


def apply_merges_to_sorting(
sorting, merge_unit_groups, new_unit_ids=None, censor_ms=None, return_extra=False, new_id_strategy="append"
):
sorting: BaseSorting,
merge_unit_groups: list[list[int | str]] | list[tuple[int | str]],
new_unit_ids: list[int | str] | None = None,
censor_ms: float | None = None,
return_extra: bool = False,
new_id_strategy: str = "append",
) -> NumpySorting | tuple[NumpySorting, np.ndarray, list[int | str]]:
"""
Apply a resolved representation of the merges to a sorting object.

Expand All @@ -245,9 +250,9 @@ def apply_merges_to_sorting(

Parameters
----------
sorting : Sorting
sorting : BaseSorting
The Sorting object to apply merges.
merge_unit_groups : list/tuple of lists/tuples
merge_unit_groups : list of lists/tuples
A list of lists for every merge group. Each element needs to have at least two elements (two units to merge),
but it can also have more (merge multiple units at once).
new_unit_ids : list | None, default: None
Expand Down
22 changes: 11 additions & 11 deletions src/spikeinterface/core/sortinganalyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -1140,18 +1140,18 @@ def remove_units(self, remove_unit_ids, format="memory", folder=None) -> "Sortin

def merge_units(
self,
merge_unit_groups,
new_unit_ids=None,
censor_ms=None,
merging_mode="soft",
sparsity_overlap=0.75,
new_id_strategy="append",
return_new_unit_ids=False,
format="memory",
folder=None,
verbose=False,
merge_unit_groups: list[list[str | int]] | list[tuple[str | int]],
new_unit_ids: list[int | str] | None = None,
censor_ms: float | None = None,
merging_mode: str = "soft",
sparsity_overlap: float = 0.75,
new_id_strategy: str = "append",
return_new_unit_ids: bool = False,
format: str = "memory",
Copy link
Member

@zm711 zm711 Mar 11, 2025

Choose a reason for hiding this comment

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

One general question that will matter for typing going forward. We have been moving toward doing:

"append" | "new" for typing but type analysis programs don't like this so I assume pydantic won't either. str however is not accurate either because it doesn't expect any string, but specific strings. So in this case should we move the library over to
Literal['append' | 'new']
I forget the actual argument so 'new' was me just making something up for example.

Or does pydantic only accept str and doesn't accept Literal?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think pydantic only accepts Literal.

Why ""append" | "new" for typing but type analysis programs don't like this"?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. On vscode I only get a warning saying that the type "append" | "new" are not defined. And others (I think Heberto) have commented about why not use Literal['append' | 'new'] so maybe he is seeing the typing warning too. I just want to make sure we fit in the pydantic model but also be useful to the end user. Saying str is not useful to the end-user that uses type hints because it is actually a Literal. I think adding Literal clutters stuff, but if we are now relying on a tool that expects Literal then we have to use it and we should move the whole code base in that direction for consistency.

I think the static type analysis programs think that "append" should be a type because we are not specifying it is a literal. So although python allows it, I think static type checkers don't know what to do with it. It is a little similar to the Optional, optional debate in type hinting in python.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think using "append" | "new" is not supported...

Copy link
Member

Choose a reason for hiding this comment

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

That is exactly what I'm saying!

I prefer it, but it is not supported. So we need to switch! I don't want us to switch to str I want us to switch to Literal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it! That makes sense to me :)

folder: Path | str | None = None,
verbose: bool = False,
**job_kwargs,
) -> "SortingAnalyzer":
) -> "SortingAnalyzer | tuple[SortingAnalyzer, list[int | str]]":
"""
This method is equivalent to `save_as()` but with a list of merges that have to be achieved.
Merges units by creating a new SortingAnalyzer object with the appropriate merges
Expand Down
Loading