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.

Sign up for GitHub

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

Fix Kilosort Phy reader docstrings #2022

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Fix Kilosort Phy reader docstrings #2022

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b2ac19
Fix Kilosort Phy reader docstrings
alejoe91 Sep 19, 2023
e3cb9bb
Typing and docstrings
alejoe91 Sep 21, 2023
b78ff8c
Merge branch 'main' into read-ks-docstring
alejoe91 Sep 21, 2023
195f03c
oups
alejoe91 Sep 21, 2023
c835547
Merge branch 'read-ks-docstring' of github.com:alejoe91/spikeinterfac…
alejoe91 Sep 21, 2023
8e3324b
List -> list
alejoe91 Sep 21, 2023
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
22 changes: 16 additions & 6 deletions src/spikeinterface/extractors/phykilosortextractors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ class BasePhyKilosortSortingExtractor(BaseSorting):
Cluster groups to exclude (e.g. "noise" or ["noise", "mua"]).
keep_good_only : bool, default: True
Whether to only keep good units.
remove_empty_units : bool, default: True
If True, empty units are removed from the sorting extractor.
load_all_cluster_properties : bool, default: True
If True, all cluster properties are loaded from the tsv/csv files.
"""

extractor_name = "BasePhyKilosortSorting"
Expand Down Expand Up @@ -197,18 +201,26 @@ class PhySortingExtractor(BasePhyKilosortSortingExtractor):
Path to the output Phy folder (containing the params.py).
exclude_cluster_groups: list or str, optional
Cluster groups to exclude (e.g. "noise" or ["noise", "mua"]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
exclude_cluster_groups: list or str, optional
Cluster groups to exclude (e.g. "noise" or ["noise", "mua"]).
exclude_cluster_groups: list or str, default: None
Cluster groups to exclude (e.g. "noise" or ["noise", "mua"]).
If None keeps all cluster groups

Did you want to start switching away from optional and include default: None in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes! let me do it since I'm at it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@zm711 should be good to go. Also added typing

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alejoe91 looks good to me. Also with the new typing (although it is a bit annoying/also very Python) that you can also do list[str] and use the built in type rather than the typing List. That's not a suggestion, it's more of a rant that sometimes a little less flexibility makes it easier for programmers to be consistent. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails with python 3.8 though! So as long as we keep support for 3.8 we should stick to this IMO :)

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. I'm sure @h-mayorquin would know :)

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Seems like it works!

Copy link
Collaborator

@zm711 zm711 Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now that's the scientist: just do the test and prove the hypothesis :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that's the scientist: just do the test and prove the hypothesis :)

I wish science was that easy!!! :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Empiricism for the win!

load_all_cluster_properties : bool, default: True
If True, all cluster properties are loaded from the tsv/csv files.

Returns
-------
extractor : PhySortingExtractor
The loaded data.
The loaded Sorting object.
"""

extractor_name = "PhySorting"
name = "phy"

def __init__(self, folder_path, exclude_cluster_groups=None):
BasePhyKilosortSortingExtractor.__init__(self, folder_path, exclude_cluster_groups, keep_good_only=False)
def __init__(self, folder_path, exclude_cluster_groups=None, load_all_cluster_properties=True):
BasePhyKilosortSortingExtractor.__init__(
self,
folder_path,
exclude_cluster_groups,
keep_good_only=False,
load_all_cluster_properties=load_all_cluster_properties,
)

self._kwargs = {
"folder_path": str(Path(folder_path).absolute()),
Expand All @@ -223,8 +235,6 @@ class KiloSortSortingExtractor(BasePhyKilosortSortingExtractor):
----------
folder_path: str or Path
Path to the output Phy folder (containing the params.py).
exclude_cluster_groups: list or str, optional
Cluster groups to exclude (e.g. "noise" or ["noise", "mua"]).
keep_good_only : bool, default: True
Whether to only keep good units.
If True, only Kilosort-labeled 'good' units are returned.
Expand All @@ -234,7 +244,7 @@ class KiloSortSortingExtractor(BasePhyKilosortSortingExtractor):
Returns
-------
extractor : KiloSortSortingExtractor
The loaded data.
The loaded Sorting object.
"""

extractor_name = "KiloSortSorting"
Expand Down