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

[NEW PROJECT]: Standardize SpikeInterface API #6

Open
3 tasks
alejoe91 opened this issue Apr 22, 2024 · 16 comments · May be fixed by #7
Open
3 tasks

[NEW PROJECT]: Standardize SpikeInterface API #6

alejoe91 opened this issue Apr 22, 2024 · 16 comments · May be fixed by #7

Comments

@alejoe91
Copy link
Member

Project title:

Key Investigators

Project Description

Ported from issue SpikeInterface/spikeinterface#2303

Following a conversion in SpikeInterface/spikeinterface#2299, a number of instances where the API is inconsistent were highlighted. This issue is opened as a place to discuss and begin fixing these:

  • ZeroChannelPaddedRecording saves it's parent recording to a parent_recording kwarg not a recording kwarg that seems to be used elsewhere.
  • ZeroSilencePeriods stores the parent recording as a dict not a recording.
  • CurationSorting uses a parent_sorting rather than sorting
  • folder vs. output_folder argument.

More genreally, I think SI is becomming a very mature and widely used package and freezing the API / defaults as much as possible between versions would really enhance user experience / avoid bugs. I wonder if at some stage a full review of the API could be performed (this would entail multiple people going through every SI function / class and making notes on function names and default arguments inconsistencies or room for improvement). Then these could be discussed, frozen, and only changed in future with a very good reason.

Background

SpikeInterface is now over 6 years old. Parts of the API have been ported and changed without a unified vision in mind.
As we're approaching v1.0, it is time to look at the API as a whole and unify the naming and conventions throughout the entire codebase.

Objectives

Unify arguments, variable naming, and documentation about the API

Approach and Plan

  • Identify API mismatches
  • Discuss unified naming

Progress

  • Make PR to SpikeInterface
@github-actions github-actions bot linked a pull request Apr 22, 2024 that will close this issue
@zm711
Copy link

zm711 commented Apr 24, 2024

The only thing I was thinking about was if the plan is to release the SortingAnalyzer before the conference it might make sense to try to do these before the release, no? That way we get a bunch of "breaking" api changes all at once again. Or would we prefer to do the deprecation style of so that we don't really break the api?

if old_arg is not None:
    warn(xxx)
    new_arg = old_arg

@JoeZiminski
Copy link

Thanks @alejoe91! Looks great, one additional aim I might add would be to identify parts of the API that developers are not happy with / think could be improved in future. Then if there is consensus these could all be changed at the same time. This would help limit the number of tweaks needed in future.

@Ashkees
Copy link

Ashkees commented May 7, 2024

As a user, I noticed that the input arguments of run_sorter and run_sorter_by_property could be standardized

@h21ak9
Copy link

h21ak9 commented May 7, 2024

My colleagues and I have noticed API inconsistencies and think this proposal is a splendid idea. I would like to contribute to this project in Edinburgh.

-Nikhil Chandra

@h-mayorquin
Copy link

Adding some data point, the functions for settings the probe, we have set_probe, set_probes and set_probe_group. The first and the third use the second, mabye that one should be private?

@h-mayorquin
Copy link

Related:
SpikeInterface/spikeinterface#1989

@h-mayorquin
Copy link

h-mayorquin commented May 7, 2024

We have get_channel_property which duplicates get_property and there is no get_unit_property. I think it should be one way or the other.

@jakeswann1
Copy link

Potentially interested in contributing during the hackathon 🙂

@zm711
Copy link

zm711 commented May 7, 2024

That's awesome @jakeswann1. Welcome aboard!

@alejoe91
Copy link
Member Author

alejoe91 commented May 9, 2024

  • Visualize/Vizualise: IMO we should stick to american english!

@zm711
Copy link

zm711 commented May 9, 2024

Great point. I'll watch out for it!

@h-mayorquin
Copy link

has_scaled vs has_scaled_traces

@h-mayorquin
Copy link

@florian6973
Copy link

Maybe between calculate and compute from compute_quality_metrics and calculate_pc_metrics

@JoeZiminski
Copy link

JoeZiminski commented May 30, 2024

@alejoe91 @samuelgarcia Related to this, I wonder your thoughts on trying and set a date to 'fix' as best possible the API. I've found that API changes are quite disruptive to user workflows and can make updating to new versions a frustratin g experience. Obviously, it is not possible to fully fix the API and never change it again, but at some stage would be nice to review the existing API, make any last changes where desired, and following this have API stability a key goal. If for certain modules it's not possible to fix at present, this could be staged (e.g. io, preprocesing, sorting have fixed API. Motionion correction, postprocessing is currently in development).

On this, it could be cool to have a kind of SpikeInterface 'roadmap', laying out what are the long-term aims of the package as well as kind of shorter-term plan. For example what are the goals for the package in the next 6 months, 1 year, 2 years? API stability could be a potential goal on this. In general a roadmap can also be useful for getting feedback on what is / isn't useful for the community. What do you think?

@zm711
Copy link

zm711 commented May 31, 2024

I would recommend we move this over to the main repo just so this doesn't get buried after the hackathon. I think making a roadmap is a great idea. I think your idea of a couple zoom sprints could also be very cool to try to push towards api stability at least at the outer level.

do you want to open the issue or you want me to copy this over for us?

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