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

Fix the bug for computing quality metrics in spikesorting v0 #1212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sytseng
Copy link

@sytseng sytseng commented Jan 17, 2025

Description

I added a bug fix for spikesorting.v0.spikesorting_curation.QualityMetrics._compute_metric described in #1202 to make sure num_spikes is included when num_spikes < min_spikes.

Checklist:

  • No. This PR should be accompanied by a release: no
  • N/a If release, I have updated the CITATION.cff
  • No. This PR makes edits to table definitions: (yes/no)
  • N/a If table edits, I have included an alter snippet for release notes.
  • N/a If this PR makes changes to position, I ran the relevant tests locally.
  • I have updated the CHANGELOG.md with PR number and description.
  • N/a I have added/edited docs/notebooks to reflect the changes

@edeno edeno requested review from khl02007 and CBroz1 January 18, 2025 01:06
@CBroz1
Copy link
Member

CBroz1 commented Jan 18, 2025

Thanks @sytseng ! I edited your description to add the template checklist. Looks like the only thing missing is adding a line-item to the CHANGELOG with this PR number, 1212

min_spikes = metric_params.get("min_spikes", 10)

for unit_id in waveform_extractor.sorting.get_unit_ids():
# checks to avoid bug in spikeinterface 0.98.2
if num_spikes[unit_id] < min_spikes:
if num_spikes[unit_id] < min_spikes and not is_num_spikes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I may be wrong about this since it's been a while since I wrote this (please correct me if that is the case). It seems we need special handling of this case (i.e. number of spikes for a unit is below min_spikes) only for computing nn_isolation and nn_overlap metrics. So instead of adding not is_num_spikes, how about changing to code to check for only those two conditions and have all the others go to ? That way we won't have to fix this again when we compute another metric (like snr, isi_violation, peak_offset etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants