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

Major refactor! Remove additional variation of the "ephys" modules! Support using SpikeInterface for spike sorting #201

Draft
wants to merge 186 commits into
base: main
Choose a base branch
from

Conversation

ttngu207
Copy link
Contributor

@ttngu207 ttngu207 commented Sep 19, 2024

  • Update - No longer support multiple variation of ephys module, keep only ephys_no_curation module, renamed to ephys
  • Update - Remove other ephys modules (e.g. ephys_acute, ephys_chronic) (moved to different branches)
  • Update - Add support for SpikeInterface
  • Update - Remove support for ecephys_spike_sorting (moved to a different branch)
  • Update - Simplify the "activate" mechanism

Accompanying this PR will be new branches created to maintain support for the other ephys module variation

  • main_ephys_chronic
  • main_ecephys_spike_sorting
  • main_ephys_precluster

For additional customization, we suggest users to fork (from the most relevant branch), and customization/modification best suited for their use-cases (e.g. ephys_chronic using the ecephys_spike_sorting flow for spike sorting)

- defaults
dependencies:
- pip
- python>=3.7,<3.11
Copy link
Member

Choose a reason for hiding this comment

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

why not 3.11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be from a while back, something broke in datajoint-python with python 3.11 (diagram or connection?)
This may have already been resolved in latest datajoint-python

Copy link
Collaborator

@MilagrosMarin MilagrosMarin Sep 19, 2024

Choose a reason for hiding this comment

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

It might be due to compatibility with the last version of SpikeInterface, 0.101+. In fact, the latest release 0.101.1 has dropped support for Python<3.9.

Copy link
Member

Choose a reason for hiding this comment

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

datajont-python should work for all Python 3.9+

@rojasgabriel
Copy link

Hello! My name is Gabriel from UCLA and I was in a workshop with you guys back in April at the DJ HQ.

I was working on incorporating the ephys element and just saw this PR. It seems like a big change. Should I wait for this to be incorporated before continuing my work? I was planning on adding the chronic ephys module that is set to be deleted from what I can tell here.

@ttngu207
Copy link
Contributor Author

Hello! My name is Gabriel from UCLA and I was in a workshop with you guys back in April at the DJ HQ.

I was working on incorporating the ephys element and just saw this PR. It seems like a big change. Should I wait for this to be incorporated before continuing my work? I was planning on adding the chronic ephys module that is set to be deleted from what I can tell here.

Hi @rojasgabriel, thanks for reaching out. I would suggest that if you can wait for another week, that'd be best. We are aiming to resolve this very soon, the code will be cleaner and developer experience should be further improved.

The chronic ephys module is removed from main, and will be moved to a different branch, stemming from main. You can work with this branch (tentatively named main_ephys_chronic).

We will keep you posted on the status here, don't hesitate the reach out to me or our team if you have any questions on using the element-array-ephys in any capacity.

@rojasgabriel
Copy link

Hi @ttngu207. Thanks for the reply and for the progress update! I'll wait a bit then before continuing with my work. I have a question now though: is the purpose of this so that every ephys use case should use the new ephys module, or should we users now just work off of branches?

@ttngu207
Copy link
Contributor Author

Hi @rojasgabriel , the main goal is two-fold

  1. Significantly simplify the repository to improve the experience for new users and developers
  • having multiple variation of ephys modules is often confusing, hard to fully document, not obvious for new users to understand which to use and how things fit together
  • these modules are not meant to be used together, users must pick one. If there are mistakes made, it's hard to roll back (for new users not too familiar with DataJoint or Database programming in general)
  • in order to support different ephys modules, the "activate" mechanism gets complicated
  1. Improve maintainability
  • with the different ephys modules, there's a lot of copied/pasted code between them, refactored these code makes the whole repo more complicated than it should be
  • hard to pull changes to all modules

Our recommendation is for users to use branches or forks, if you need heavy customization. In fact, forking has been the the practice for many users already (we're just not formalizing it here yet). Branches or forks also follow Git/Github standard development practice.

@tabedzki
Copy link
Contributor

@ttngu207, how different will this be compared to the old format in terms of actual tables? I'd love to be able to rebase and stay up to date but not if it breaks everything.

@ttngu207
Copy link
Contributor Author

@ttngu207, how different will this be compared to the old format in terms of actual tables? I'd love to be able to rebase and stay up to date but not if it breaks everything.

Hi @tabedzki , schema/table design is mostly the same, we expect to maintain backward compatibility at the schema design level. But I suspect there will be some backward incompatibility issues with regards to modules/functions organization (especially if you're using the ephys_preclustering module).

It would be great if we can have a quick chat about your specific use case to make sure nothing breaks in a major way.

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.

8 participants