Skip to content

Start again openephys binary rawio. #945

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.

Sign up for GitHub

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

Jump to bottom
Merged
JuliaSprenger merged 23 commits into from
Mar 26, 2021
Merged

Start again openephys binary rawio. #945

JuliaSprenger merged 23 commits into from
Mar 26, 2021

Conversation

@samuelgarcia
Copy link
Contributor

@samuelgarcia samuelgarcia commented Feb 17, 2021

new implementation of this : #908

@JuliaSprenger : this is an empty implementation we can start on.

@pep8speaks
Copy link

pep8speaks commented Feb 17, 2021 •
edited
Loading

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

Line 19:100: E501 line too long (126 > 99 characters)
Line 20:100: E501 line too long (130 > 99 characters)
Line 21:100: E501 line too long (126 > 99 characters)
Line 22:100: E501 line too long (124 > 99 characters)
Line 23:100: E501 line too long (124 > 99 characters)
Line 24:100: E501 line too long (124 > 99 characters)
Line 25:100: E501 line too long (124 > 99 characters)
Line 26:100: E501 line too long (124 > 99 characters)
Line 31:100: E501 line too long (134 > 99 characters)
Line 32:100: E501 line too long (126 > 99 characters)
Line 33:100: E501 line too long (122 > 99 characters)
Line 34:100: E501 line too long (125 > 99 characters)
Line 35:100: E501 line too long (112 > 99 characters)
Line 36:100: E501 line too long (118 > 99 characters)
Line 37:100: E501 line too long (116 > 99 characters)
Line 38:100: E501 line too long (106 > 99 characters)
Line 39:100: E501 line too long (110 > 99 characters)
Line 40:100: E501 line too long (106 > 99 characters)
Line 41:100: E501 line too long (104 > 99 characters)
Line 42:100: E501 line too long (117 > 99 characters)
Line 43:100: E501 line too long (119 > 99 characters)
Line 44:100: E501 line too long (119 > 99 characters)
Line 45:100: E501 line too long (117 > 99 characters)
Line 46:100: E501 line too long (117 > 99 characters)
Line 47:100: E501 line too long (119 > 99 characters)
Line 48:100: E501 line too long (119 > 99 characters)
Line 49:100: E501 line too long (117 > 99 characters)
Line 50:100: E501 line too long (108 > 99 characters)
Line 51:100: E501 line too long (112 > 99 characters)
Line 52:100: E501 line too long (108 > 99 characters)
Line 53:100: E501 line too long (106 > 99 characters)
Line 54:100: E501 line too long (106 > 99 characters)
Line 55:100: E501 line too long (106 > 99 characters)
Line 59:100: E501 line too long (114 > 99 characters)
Line 60:100: E501 line too long (120 > 99 characters)
Line 61:100: E501 line too long (118 > 99 characters)
Line 62:100: E501 line too long (121 > 99 characters)
Line 63:100: E501 line too long (108 > 99 characters)
Line 64:100: E501 line too long (108 > 99 characters)
Line 67:100: E501 line too long (114 > 99 characters)
Line 68:100: E501 line too long (120 > 99 characters)
Line 69:100: E501 line too long (118 > 99 characters)
Line 70:100: E501 line too long (121 > 99 characters)
Line 71:100: E501 line too long (108 > 99 characters)
Line 72:100: E501 line too long (108 > 99 characters)
Line 75:100: E501 line too long (114 > 99 characters)
Line 76:100: E501 line too long (120 > 99 characters)
Line 77:100: E501 line too long (118 > 99 characters)
Line 78:100: E501 line too long (121 > 99 characters)
Line 79:100: E501 line too long (108 > 99 characters)
Line 80:100: E501 line too long (108 > 99 characters)
Line 83:100: E501 line too long (114 > 99 characters)
Line 84:100: E501 line too long (120 > 99 characters)
Line 85:100: E501 line too long (118 > 99 characters)
Line 86:100: E501 line too long (121 > 99 characters)
Line 87:100: E501 line too long (108 > 99 characters)
Line 88:100: E501 line too long (108 > 99 characters)
Line 91:100: E501 line too long (114 > 99 characters)
Line 92:100: E501 line too long (120 > 99 characters)
Line 93:100: E501 line too long (118 > 99 characters)
Line 94:100: E501 line too long (121 > 99 characters)
Line 95:100: E501 line too long (108 > 99 characters)
Line 96:100: E501 line too long (108 > 99 characters)
Line 99:100: E501 line too long (114 > 99 characters)
Line 100:100: E501 line too long (120 > 99 characters)
Line 101:100: E501 line too long (118 > 99 characters)
Line 102:100: E501 line too long (121 > 99 characters)
Line 103:100: E501 line too long (108 > 99 characters)
Line 104:100: E501 line too long (108 > 99 characters)

Comment last updated at 2021-03-23 07:48:31 UTC

This was referenced Feb 17, 2021
Closed
Closed
@apdavison apdavison added this to the 0.10.0 milestone Mar 4, 2021
@samuelgarcia
Copy link
Contributor Author

samuelgarcia commented Mar 12, 2021

Hi Julia.
I am back on this:

  • integration in rawio API
  • array annoattions for signals and events.

@samuelgarcia
Copy link
Contributor Author

samuelgarcia commented Mar 12, 2021

@JuliaSprenger
We have a bug with array annoations and possible types.
Could you have a look ?

samuelgarcia
samuelgarcia commented Mar 15, 2021
View reviewed changes
JuliaSprenger
JuliaSprenger reviewed Mar 15, 2021
View reviewed changes
samuelgarcia
samuelgarcia commented Mar 15, 2021
View reviewed changes
@JuliaSprenger
Copy link
Member

JuliaSprenger commented Mar 16, 2021

@JuliaSprenger
We have a bug with array annoations and possible types.
Could you have a look ?

I had a look and implemented an unwrapping of the _npy metadata provided for events. Do you think this might also occur for other signal types?

@samuelgarcia
Copy link
Contributor Author

samuelgarcia commented Mar 16, 2021

No I think this is only for events.

@JuliaSprenger JuliaSprenger mentioned this pull request Mar 18, 2021
Closed
@samuelgarcia samuelgarcia marked this pull request as ready for review March 18, 2021 10:22
JuliaSprenger
JuliaSprenger reviewed Mar 18, 2021
View reviewed changes
JuliaSprenger
JuliaSprenger reviewed Mar 18, 2021
View reviewed changes
neo/rawio/openephysbinaryrawio.py
# sync_timestamps = np.load(str(sync_timestamp_file), mmap_mode='r')
# t_start = sync_timestamps[0]

# TODO for later : gap checking
Copy link
Member

@JuliaSprenger JuliaSprenger Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do we leave this until we encounter a test file that contains a gap?

JuliaSprenger
JuliaSprenger reviewed Mar 18, 2021
View reviewed changes
neo/rawio/openephysbinaryrawio.py

all_streams[block_index][seg_index]['events'][stream_name] = event_stream

# TODO for later: check stream / channel consistency across segment
Copy link
Member

@JuliaSprenger JuliaSprenger Mar 18, 2021

Choose a reason for hiding this comment

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

This would be nice to have.

JuliaSprenger
JuliaSprenger approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@JuliaSprenger JuliaSprenger left a comment

Choose a reason for hiding this comment

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

Only a few typo corrections.

neo/rawio/openephysbinaryrawio.py
return sigs

def _spike_count(self, block_index, seg_index, unit_index):
pass
Copy link
Member

@JuliaSprenger JuliaSprenger Mar 18, 2021

Choose a reason for hiding this comment

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

Wouldn't it be safer to raise a NotImplementedError here in case someone starts extending the IO for handling spikes also?

@JuliaSprenger
Copy link
Member

JuliaSprenger commented Mar 19, 2021

@samuelgarcia Do you agree with my suggestions? I think we could merge this in any case soon, just accept / reject my suggestions.

@samuelgarcia
Copy link
Contributor Author

samuelgarcia commented Mar 23, 2021

I merge all your last modification. If test pass then we can merge no ?

@samuelgarcia
Copy link
Contributor Author

samuelgarcia commented Mar 26, 2021

Hi Julia,
can we merge this ?

@JuliaSprenger JuliaSprenger merged commit bc60370 into NeuralEnsemble:master Mar 26, 2021
@samuelgarcia
Copy link
Contributor Author

samuelgarcia commented Mar 26, 2021

merci!!

@samuelgarcia samuelgarcia deleted the openephys_binary branch March 9, 2022 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JuliaSprenger JuliaSprenger JuliaSprenger approved these changes

Assignees

@JuliaSprenger JuliaSprenger

Labels

None yet

Projects

None yet

Milestone

0.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@samuelgarcia @pep8speaks @JuliaSprenger @apdavison