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

Update spike sorting V1 code and notebook to run sorters other than mountainsort4 #844

Merged
merged 18 commits into from
Mar 7, 2024

Conversation

donghoon-shin
Copy link
Contributor

@donghoon-shin donghoon-shin commented Feb 20, 2024

Description

  • src/spyglass/spikesorting/v1/sorting.py : modifying sorter_params dictionary ; spikeinterface.run_sorter parameters based on spikesorting algorithms.
  • notebooks/10_Spike_SortingV1.ipynb: Change a few notebook cells so that it can run without error until spike sorting. Also added more instructions on running other spikesorting algorithms such as kilosort2.5, kilosort3, and mountainsort5.

Checklist:

  • This PR should be accompanied by a release: (unsure)
  • (If release) I have updated the CITATION.cff
  • I have updated the CHANGELOG.md
  • I have added/edited docs/notebooks to reflect the changes

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@edeno edeno requested review from khl02007 and CBroz1 February 20, 2024 07:06
Copy link
Member

@CBroz1 CBroz1 left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions @donghoon-shin !

I have a couple code comments on black, pop, and sorter name validation. Having the py-script of the notebook (see how here) would make it easier to give comments on those changes. In particular, it looks like you left some things commented out, which might make it hard to run outside the lab's DB. I think I would put these in if clauses to avoid w/e error you intend to avoid by commenting out

@donghoon-shin
Copy link
Contributor Author

@CBroz1 I fixed as your comments except for the ones that I commented. Please let me know!

@edeno edeno requested a review from CBroz1 February 21, 2024 10:45
Copy link
Member

@CBroz1 CBroz1 left a comment

Choose a reason for hiding this comment

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

Thanks for converting the notebook @donghoon-shin !

I had some comments about comments there, and then extracting common kwargs outside of the if/then.

It took me a sec to find your comment. I'm in the habit of leaving items unresolved until both author and reviewer are on the same page

Thanks again for the contribution!


sgc.LabMember.insert_from_nwbfile(nwb_file_name)
# +
# sgc.LabMember.insert_from_nwbfile(nwb_file_name)
Copy link
Member

Choose a reason for hiding this comment

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

Please uncomment and put in some form of conditional like

Suggested change
# sgc.LabMember.insert_from_nwbfile(nwb_file_name)
if not (sgc.LabMember & 'lab_member_name = "Firstname Lastname"'):
sgc.LabMember.insert_from_nwbfile(nwb_file_name)

Jupytext makes it easier to comment on specific lines, but it can be inconsistent in the py -> ipynb direction, so I unfortunately generally apply comments by hand in the notebook.

@@ -52,33 +54,54 @@
# insert SortGroup
#

sgs.SortGroup.set_group_by_shank(nwb_file_name=nwb_file_name2)
# +
# sgs.SortGroup.set_group_by_shank(nwb_file_name=nwb_file_name2)
Copy link
Member

Choose a reason for hiding this comment

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

Please either set this in a conditional or provide some context about why this is commented out. e.g., a markdown cell saying "If you haven't already done X, please uncomment the line below"

@edeno edeno requested a review from CBroz1 February 22, 2024 09:15
Copy link
Member

@CBroz1 CBroz1 left a comment

Choose a reason for hiding this comment

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

Approved pending resolution of @edeno's comment re #592

@khl02007
Copy link
Collaborator

@donghoon-shin can you make the changes to the notebook and verify that everything runs?

@khl02007
Copy link
Collaborator

The notebook needs to be cleaned up more. I have started adding some more documentation to make it more thorough.

@khl02007 khl02007 changed the title Testing/modifying to run mountainsort5; kilosort2.5 and kilosort3 with GPU; Fixing spikesorting(v1) tutorial notebook to run without error Change spike sorting V1 code and notebook to run sorters other than mountainsort4 Feb 29, 2024
@khl02007 khl02007 changed the title Change spike sorting V1 code and notebook to run sorters other than mountainsort4 Update spike sorting V1 code and notebook to run sorters other than mountainsort4 Feb 29, 2024
@edeno
Copy link
Collaborator

edeno commented Mar 7, 2024

In conversation with @khl02007 , this is good to go. There will be a followup PR that will do some notebook cleanup.

@edeno edeno merged commit 647a5f8 into LorenFrankLab:master Mar 7, 2024
7 checks passed
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.

4 participants