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
FIX+ENH: Fix indexing in events and add measurement date option to writer #29
Conversation
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
==========================================
+ Coverage 94.89% 95.37% +0.47%
==========================================
Files 3 3
Lines 196 216 +20
==========================================
+ Hits 186 206 +20
Misses 10 10
Continue to review full report at Codecov.
|
meas_date : str | None | ||
The measurement date of the data specified as a string in the format: | ||
"YYYYMMDDhhmmssuuuuuu". "u" stands for microseconds. If None, defaults | ||
to '00000000000000000000'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not allow for ISO 8601 formatting which is a bit more human-readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I am now allowing datetime.datetime
objects, I think this should cater to all needs :-)
See: d0467c2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small comment but in general this looks good to me, thanks for tackling this bug
Thanks for the review @choldgraf All tests pass, I am merging this now and preparing a new release :-) |
This PR does two things
1
This PR exposes a
meas_date
argument to the writer function and a test suite. The default isNone
and maintains the current behavior.2
This PR fixes #27
I am adding
1
to each sample index, to account for BrainVision's 1-based indexing (whereas we expect an array that is 0-based indexed). Note, I changed the docstring forevents
. It now mentions that we expect a zero-based indexed array of events.Currently the reasoning that BrainVision uses 1-based indexing is based on inspecting files, and realizing that the "New Segment" marker is always the first one, and always at sample
1
... not at0