Skip to content

Standardising parts of the API. #2303

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

Open
JoeZiminski opened this issue Dec 6, 2023 · 11 comments
Open

Standardising parts of the API. #2303

JoeZiminski opened this issue Dec 6, 2023 · 11 comments
Labels
packaging Related to packaging/style

Comments

@JoeZiminski
Copy link
Collaborator

Following a conversion in #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. I wonder what people think about this.

@alejoe91
Copy link
Member

alejoe91 commented Dec 6, 2023

Thanks for this write up, @JoeZiminski (and @zm711 ).

I agree that we should unify the API wherever we can. For folder VS output_folder, let's keep it in mind when starting with #2282 (I guess that is mainly for the WaveformExtractor, right?)

@alejoe91 alejoe91 added the packaging Related to packaging/style label Dec 6, 2023
@zm711
Copy link
Member

zm711 commented Dec 6, 2023

run_sorter and run_sorter_by_property use different folder arguments.

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Dec 6, 2023

So what should be for preprocessing? parent_recording or justrecording?

@zm711
Copy link
Member

zm711 commented Dec 6, 2023

Most function/classes in the spikeinterface have a positional argument labeled as recording (so I switched the docs to use keyword to encourage people to use the keyword instead), so the path of least resistance (and most consistent for people) would be to use recording and sorting for all functions which take those values in. Rather than having some rare uses of parent_recording/sorting.

@h-mayorquin
Copy link
Collaborator

I actually do find some value on the fact that parent_recording differentiates from the (current)_recording which is usually self. On the other hand we also use the same term for the Segments so it is kind of overloaded:

https://github.com/catalystneuro/spikeinterface/blob/6fabd004ddbe6a03df5897de4cb2a8860487de36/src/spikeinterface/core/base.py#L1172-L1182

I would rather use input_recording in preprocessing as a general thing but maybe only using recording is fine. Just wanted to point out some positive aspect for disambiguation.

@zm711
Copy link
Member

zm711 commented Dec 6, 2023

I would be fine if every preprocessor took in parent_recording (or input_recording), but since so much code could be using the keyword recording at this point it seems more disruptive to use a different keyword. But from a mental schema I think using something other than recording could actually be beneficial.

@JoeZiminski
Copy link
Collaborator Author

I agree parent_recording or input_recording is a better name as recording on a Recording that is not self is confusing. It is worth collecting examples of such potential renaming improvements, and these can be changed together at an opportune moment sometime later. I think it's okay to make a change in a suboptimal direction for consistency now with an eye to making a more disruptive change to a better name in future.

@JoeZiminski
Copy link
Collaborator Author

duration vs. durations in the test data generation suite

@JoeZiminski
Copy link
Collaborator Author

Hey @alejoe91 @zm711 @h-mayorquin I thought I'd resurrect this thread considering (as I understand it) the current work on #2282 is being wrapped up and docs being written. I wonder if it is even worth spending an afternoon (or possible hackathon for the meet-up) going through the entire API and categorising function names / arguments into:

  • happy with
  • unhappy with but will live with and won't change in future (to avoid changing too much)
  • unhappy with and will change now

However this is a lot of work so appreciate it might not be a priority at the moment or might be better later as a standalone endeavour-if SortingAnalyzer changes are already complicated adding a second load of API renaming on top might be too much!

@zm711
Copy link
Member

zm711 commented Apr 19, 2024

I like that @JoeZiminski ! I can think a little API polish might be nice :)

@alejoe91
Copy link
Member

Hackathon project?

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

No branches or pull requests

4 participants