Skip to content

Conversation

@JuliaSprenger
Copy link
Member

@JuliaSprenger JuliaSprenger commented Apr 30, 2021

This PR implements the required rawio methods for reading spiketimes and waveforms. It also restructures the IO to be able to support multi-file datasets (.bin & unit files).

This PR depends on the Axona unit test data that will be added to the ephy_testing_data: https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/49/files

@pep8speaks
Copy link

pep8speaks commented Apr 30, 2021

Hello @JuliaSprenger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-18 12:16:53 UTC

Copy link
Contributor

@sbuergers sbuergers left a comment

Choose a reason for hiding this comment

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

Very nicely done. I mostly have nit-picky comments. I will send you the sample data so we can run proper tests with neo (should then also include the filenames in the rawio and io tests of axona).

@JuliaSprenger JuliaSprenger marked this pull request as ready for review May 6, 2021 10:50
@JuliaSprenger
Copy link
Member Author

@samuelgarcia Do you have any comments for this PR? I already rebased to include the latest changes in the test suite.

@JuliaSprenger
Copy link
Member Author

I added more testfiles to gin (https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/54). @samuelgarcia can you check these and merge both PRs if you are happy with the current state?

@apdavison apdavison modified the milestone: 0.10.0 May 10, 2021
@apdavison apdavison added the IO label May 10, 2021
@JuliaSprenger
Copy link
Member Author

Hi @samuelgarcia it seems like the new test data files are not automatically downloaded from gin. Can we do a manual reset of the cache?

@JuliaSprenger JuliaSprenger mentioned this pull request May 11, 2021
@samuelgarcia
Copy link
Contributor

Weird. I will have a look.
Even with the cache the file should be retreive if not present.
What happen on local mahcine ?

@JuliaSprenger
Copy link
Member Author

I observed the same effect locally. The new files were not automatically downloaded and I had to delete the complete ephys_testing_data folder to trigger a new download.

@samuelgarcia
Copy link
Contributor

STrange. I will have a look.

Copy link
Contributor

@sbuergers sbuergers left a comment

Choose a reason for hiding this comment

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

I think with this patch it should be good now (besides the gin issues mentioned above):

# spike times are repeated for each contact -> use only first contact
unit_spikes = raw_spikes['spiketimes'][::4]

# slice spike times only if needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, when t_start and t_stop are given the whole timeseries is returned.

Only specific segments are returned when either t_start or t_stop is None.

I made a local patch for this, and will still be working with it today. If it works fine I will either just send it to you or put it in the catalystneuro fork.

@JuliaSprenger
Copy link
Member Author

@samuelgarcia Any news regarding the download of the new test data?

@JuliaSprenger
Copy link
Member Author

@samuelgarcia I cherry-picked your datalad fix also in this branch, so now all files are downloaded properly and tests pass.
Ready for final review / merge from my side.

@JuliaSprenger
Copy link
Member Author

JuliaSprenger commented Jun 11, 2021

Hi @samuelgarcia with the fetching of the testfiles fixed and this PR being in a completish state for some time already it's up to your final approval for this to be merged!

sbuergers and others added 19 commits June 18, 2021 13:32
…arawio (rather than depend on spikeextractors)
This reverts commit 0340f4f.
This reverts commit 5b1b934.
… to axonarawio (rather than depend on spikeextractors)"

This reverts commit cbed9f3.
…for get_analogsignal_chunk when no .bin file"

This reverts commit cccb739.
@JuliaSprenger
Copy link
Member Author

I had to rebase on master to resolve conflicts with base, but now all comments are addressed. @samuelgarcia You can either provide more feedback or merge.

@samuelgarcia
Copy link
Contributor

This looks good.
Do you want I merge now ?
What conflit you want to fix ?

@JuliaSprenger
Copy link
Member Author

I am ready to merge. @samuelgarcia go ahead ;)

@samuelgarcia samuelgarcia merged commit 3d8b139 into NeuralEnsemble:master Jun 21, 2021
@JuliaSprenger JuliaSprenger deleted the enh/axona_spikes branch April 3, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants