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

Add 'How SpikeInterface handles time' documentation #3072

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

Conversation

JoeZiminski
Copy link
Collaborator

@JoeZiminski JoeZiminski commented Jun 24, 2024

(The documentation as rewritten since first posted. See the drop down for original PR message).

This PR updates the existing tutorial for times here to highlight some of the new time handling methods.

Currently the section on 'shift_start_times' doesn't exist but I wrote this doc to see how this API would feel, so it is a kind of prototype docs for the new method on #3509. This PR will await #3509. The build docs are here.

TODO:

  • sample_index_to_to_timeandtime_to_sample_index` are currently not in the API docs
Old PR message - check all sections that are not running, because they are below the line that crashes because #3509 function doesn't exist.
Old PR message

In-progress draft of a page detailing how time is represented in SpikeInterface. closes #2627. Once a first draft is finished will require a lot of revision I'm sure as I'm not certain on a lot of this.

Worth discussion is where this goes, at present is a How-to but it is a little tenuous. Maybe it can be reworded to 'How to handle time on SpikeInterface recordings' with a bit more code added.

(An aside, please excuse if there already solutions to this I have missed). As the documentation grows API changes will frequently break existing documentation. One solution is to add a template to all PR that has a tick-box stating that all relevant documentation must be reviewed and updated before merge. This is good practice anyway but is error-prone. There seem to be some existing solutions to testing code snippets within documentation: pylit[https://pypi.org/project/pylit/] sphinx. Actually I see from searching @h-mayorquin might be able to advise on this!

EDIT: some things to do before continuing

  • currently I don't think it is possible to manually set t_start on a recording object. Instead it is only loaded from certain extractors. As far as I can tell. It would be cool to make this accessible, this would then make the tutorial make more sense.
  • currently generate_ground_truth_recording has no t_start properly and it is not possible to set times that then relate to the sorting. Would be useful to add this before continuing here. discussed in Adding time vector to generate_ground_truth_recording() #3071.

@h-mayorquin
Copy link
Collaborator

You can find PR templates here:

https://github.com/NeurodataWithoutBorders/pynwb/tree/dev/.github/PULL_REQUEST_TEMPLATE

But let's discuss this as a separate issue. I personally don't really like them tbh but I think they are mostly good especially for the issues.

Regarding the where this should go, my two cents: I read your draft and I think this is clearly a tutorial. You are trying to teach users how the library does something, not how to solve a specific problem.

@alejoe91 alejoe91 added the documentation Improvements or additions to documentation label Jun 26, 2024
@JoeZiminski JoeZiminski changed the title Add 'How SpikeInerface handles time' documentation Add 'How SpikeInterface handles time' documentation Jun 26, 2024
@JoeZiminski JoeZiminski force-pushed the add_handling_times_documentation branch from abb4cf5 to 4ef2b69 Compare November 1, 2024 14:00
@JoeZiminski JoeZiminski force-pushed the add_handling_times_documentation branch from 05943aa to 1de97e0 Compare November 1, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Understanding how Spikeinterface handles recording times.
3 participants