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

buffer description api in neorawio and xarray reference API bridge #1513

Merged
merged 31 commits into from
Oct 25, 2024

Conversation

samuelgarcia
Copy link
Contributor

@samuelgarcia samuelgarcia commented Jul 26, 2024

Given the idea of @bendichter and this gist

This is an implementation of "buffer_description_api" for reading analogsignal chunk.
This 100% backward compatible.

The idea/goal is:

  • have a simple buffer description for analogsignal for raw binary (np.memmap) or hdf5 cases (maybe more)
  • this description can be exported to json
  • this description can be easily transformed to the xarray/zarr reference external API
  • make so file cloud ready for streaming chunk of traces

See also kerchunk

neo/rawio/xarray_utils.py Outdated Show resolved Hide resolved
neo/rawio/xarray_utils.py Outdated Show resolved Hide resolved
@bendichter
Copy link
Contributor

Looks good!

@apdavison apdavison added this to the future milestone Jul 26, 2024
…_buffer_description_api=True

This should also solve the memmap and memmory leak problem.
doc/source/rawio.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.

doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
Copy link
Contributor

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

I did a first reading. I think that this PR and the future documentation would benefit greatly if we have the schema of the buffer description somewhere. It does not have to be a formal schema (although that would be great) it could be just a description on the documentation or a python data class with types. Something that I can reference to see what should I expect to fill when I am doing a buffer like this.

For reading analog signals **neo.rawio** has 2 important concepts:

1. The **signal_stream** : it is a group of channels that can be read together using :func:`get_analog_signal_chunk()`.
This group of channels is guaranteed to have the same sampling rate, and the same duration per segment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have logical channels, this should be same units as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment not yet. The API do not enforce and ensure this.
See this https://github.com/NeuralEnsemble/python-neo/blob/master/neo/rawio/baserawio.py#L118
Ideally units should be add but some IO are mixing maybe the units in the same stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add it as an ideal? Or would you rather first do the changes and then change it here?

I can close this for example:

#1133

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some tricky cases where the units is manually done by the user for some formats.
not sure but maybe spike2 is in that case.
What should we do ? split all groups of same units in separate streams ? this could be an good idea but we need first a deeper check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. I think that the concept of stream should include same unit across the stream but this we will need to solve in a case by case basis (the deeper check that you mention).

doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Outdated Show resolved Hide resolved
doc/source/rawio.rst Show resolved Hide resolved
neo/rawio/axonrawio.py Outdated Show resolved Hide resolved
neo/rawio/baserawio.py Outdated Show resolved Hide resolved
neo/rawio/baserawio.py Outdated Show resolved Hide resolved
neo/rawio/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

couple more comments from me too.

doc/source/rawio.rst Show resolved Hide resolved
neo/rawio/axonrawio.py Show resolved Hide resolved
neo/rawio/baserawio.py Outdated Show resolved Hide resolved
neo/rawio/baserawio.py Show resolved Hide resolved
neo/rawio/brainvisionrawio.py Outdated Show resolved Hide resolved
@samuelgarcia
Copy link
Contributor Author

Merci beaucoup @zm711 and @h-mayorquin for the review

samuelgarcia and others added 3 commits October 21, 2024 14:09
Co-authored-by: Heberto Mayorquin <h.mayorquin@gmail.com>
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
@samuelgarcia samuelgarcia changed the title Proof of concept of "buffer_description_api" and xarray reference API bridge buffer description api in neorawio and xarray reference API bridge Oct 23, 2024
@samuelgarcia
Copy link
Contributor Author

@zm711 @h-mayorquin @alejoe91
This is OK on my side.
test are passing I think we can merge this soon.
we will be able to update stream and buffer on reader per reader basis later.

@zm711 zm711 modified the milestones: future, 0.14.0 Oct 25, 2024
@zm711 zm711 merged commit 96a28af into NeuralEnsemble:master Oct 25, 2024
3 checks passed
@samuelgarcia
Copy link
Contributor Author

yeah!!
merci fois pour l'aide.

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.

5 participants