Skip to content

Fix reading blackrock digital events.#552

Merged
JuliaSprenger merged 1 commit intoNeuralEnsemble:masterfrom
histedlab:mh/blackrock_digital
Sep 11, 2018
Merged

Fix reading blackrock digital events.#552
JuliaSprenger merged 1 commit intoNeuralEnsemble:masterfrom
histedlab:mh/blackrock_digital

Conversation

@histed
Copy link
Contributor

@histed histed commented Jul 19, 2018

Blackrock specs say packet_insertion_reason is
1 for PARALLEL and 129 for SERIAL. (also 64 for PERIODIC, not implemented)
i.e. bit 1 is set for parallel and bits 1 and 7 are set for serial.

Because the bitfield is just a unique ID, I removed the bit testing and use equality instead.


Apologies: I didn't write tests for this code. But this fixes a bug where both parallel and serial events were being returned as parallel due to the bit testing above.

Blackrock specs say packet_insertion_reason is
1 for PARALLEL and 129 for SERIAL.  (also 64 for PERIODIC, not implemented)
i.e. bit 1 is set for parallel and bits 1 and 7 are set for serial.

Because the bitfield is just a unique ID, I removed the bit testing and use equality instead.
@pep8speaks
Copy link

Hello @histed! Thanks for submitting the PR.

Line 347:30: E127 continuation line over-indented for visual indent
Line 889:25: E127 continuation line over-indented for visual indent
Line 1699:28: E127 continuation line over-indented for visual indent
Line 1700:28: E127 continuation line over-indented for visual indent
Line 1857:66: E231 missing whitespace after ','
Line 1858:100: E501 line too long (134 > 99 characters)

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.0%) to 49.944% when pulling 591e156 on histedlab:mh/blackrock_digital into 7753fb2 on NeuralEnsemble:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jul 19, 2018

Coverage Status

Coverage decreased (-6.0%) to 49.944% when pulling 591e156 on histedlab:mh/blackrock_digital into 7753fb2 on NeuralEnsemble:master.

@samuelgarcia
Copy link
Contributor

Thank you for the patch.
I look good.
I don't have read blacrock spec.
I let @JuliaSprenger @bjoern1001001 or @mdenker check this.
Best

@muellerbjoern
Copy link
Contributor

Hi @histed,

thank you for your contribution.

The Blackrock manual that I have available is quite ambiguous regarding this issue.
I talked to @JuliaSprenger and we both think what you did makes more sense than the current implementation.

I would only suggest a small change: Instead of raising an error when an unknown event code is found, a warning would fit better. Then the user knows there might be more data in the file that cannot be processed, but the rest of the content remains available.

@JuliaSprenger
Copy link
Member

I agree the manual is ambiguous and @histed interpretation makes more sense. @histed: Could you change the ValueError to a warning?

@JuliaSprenger JuliaSprenger added this to the 0.7.0 milestone Jul 26, 2018
@JuliaSprenger JuliaSprenger merged commit 00d94e5 into NeuralEnsemble:master Sep 11, 2018
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.

6 participants