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

Phy sorting extractor post-curation: noise and mua clusters are not filtered out #3571

Open
Djoels opened this issue Dec 5, 2024 · 3 comments
Labels
extractors Related to extractors module

Comments

@Djoels
Copy link

Djoels commented Dec 5, 2024

When setting exclude_cluster_groups = ["noise", "mua"] using read_phy or PhySortingExtractor, the units with a KSlabel of noise or MUA are still retained, if in the group column, there is no value given.
image

I assume this means the curator has finished the session considering all remaining units are MUA and thus can automatically inherit the KSlabel. However, this means that the filter used in the Phy will not filter out these NaN values:

if exclude_cluster_groups is not None:
  if isinstance(exclude_cluster_groups, str):
      cluster_info = cluster_info.query(f"group != '{exclude_cluster_groups}'")
  elif isinstance(exclude_cluster_groups, list):
      if len(exclude_cluster_groups) > 0:
          for exclude_group in exclude_cluster_groups:
              cluster_info = cluster_info.query(f"group != '{exclude_group}'")

If this is intended use, the easy fix would be to have
cluster_group["group"] = cluster_group["group"].fillna(cluster_group["KSlabel"])

Will happily create a PR for this if deemed appropriate.

@alejoe91
Copy link
Member

alejoe91 commented Dec 5, 2024

Hi, KSLabel and Phy quality are different properties. This is because we can launch Phy on any sorting output, so it's not Kilosort specific. If you want to use the KSLabel, you can easily load the property externally and set the remaining ones :)

@Djoels
Copy link
Author

Djoels commented Dec 5, 2024

Oh, my bad, since the underlying extractor was called BasePhyKilosortExtractor, I thought that I could infer it being specific to a Phy curated Kilosort output.
It seems a bit unintended that these missing values are not filtered out. Maybe a warning could help the user in realising this?
If not appropriate I will close this issue and deal with the matter outside spikeinterface.

@zm711
Copy link
Collaborator

zm711 commented Dec 6, 2024

Oh, my bad, since the underlying extractor was called BasePhyKilosortExtractor, I thought that I could infer it being specific to a Phy curated Kilosort output.

The name was just because both Phy and Kilosort have the same files (since the teams developed them together KS tries to always write files that can be read by Phy). So we can use the same base for reading the files since they are all the same. :)

It seems a bit unintended that these missing values are not filtered out. Maybe a warning could help the user in realising this?

In general we want to be as agnostic as possible for these types of things. So what Alessio was saying is you can load the property (which we totally support) and then interact with those properties yourself. Because phy labels are the "final choices" of the user for their manual curation (with any spike-sorter since we support exporting any spike sorting format to phy) we privilege those labels as something the user intends. The KS labels we load, but then expect the user to interact with them if they want to use them.

We also have tons of our own curation/qc tools to help users decide what to keep or toss. :)

Alessio can correct me if he disagrees with anything I've said, but this is how I would think about it.

@alejoe91 alejoe91 added the extractors Related to extractors module label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extractors Related to extractors module
Projects
None yet
Development

No branches or pull requests

3 participants