-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix read_raw_eyelink() failure when recording blocks are empty or starting with empty values
#13571
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
9f2d92c to
911310e
Compare
read_raw_eyelink() failure when status column starts with empty values read_raw_eyelink() failure when recording blocks are empty or starting with empty values
|
thanks @qian-chu ! And thanks in advance for your patience, just getting back to the office after the holiday break. I will take a closer look this week. |
No worries! Not expecting people to be back from holidays this soon. This PR is also still in progress anyways (I have yet to add new tests) |
just a note 'pupil' channels in Eyelink measure pupil size (or diameter), not positions, so the warning message should be 'pupil size data detected', if a warning is needed |
This PUPIL unit, if I'm not mistaken, indicates the user used |
Agreed. It is easy to mix up, but, In EyeLink systems there is a distinction between Pupil Size data and eyegaze pupil position data. Per the EDF2ASC Manual:
And per section
@qian-chu I'm only loosely following this so feel free to ping me when you are ready for review or need my input! |
|
@scott-huberty Absolutely! I think it will be helpful if you could look at the evolution of conversation in #13550. It seems to have been solved by one of your previous PR but would love to hear your opinion. This will help determine the scope of this PR. |
| fp.write("END\t7453390\tRIGHT\tSAMPLES\tEVENTS\n") | ||
| fp.write("END\t7453490\tRIGHT\tSAMPLES\tEVENTS\n") |
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.
This seems to be a small typo, because the timestamps are from 7453390 to 7453490, therefore block end message should be 7453490
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.
thanks!
| assert raw.annotations.description[-7] == "BAD_ACQ_SKIP" | ||
| assert np.isclose(raw.annotations.onset[-7], 1.001) | ||
| assert np.isclose(raw.annotations.duration[-7], 0.1) | ||
| assert raw.annotations.description[-8] == "BAD_ACQ_SKIP" | ||
| assert np.isclose(raw.annotations.onset[-8], 1.001) | ||
| assert np.isclose(raw.annotations.duration[-8], 0.1) |
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.
Event indices all move "up" by one becase a new "BAD_ACQ_SKIP" is introduced between block 2 and 3.
|
This PR is ready for review! While I believe this should resolve the bug already, it also opens a new question: what should MNE do with empty recordings? The behavior currently seems to be still read the empty data in, but maybe it makes more sense to also denote the period as "BAD_ACQ_SKIP"? (which is currently only done to between-block gaps.) Open to ideas and implementations. |
Reference issue (if any)
Fixes #13567 and extend testing to the case mentioned in #13550.
What does this implement/fix?
As described in #13567, when a recording block starts with missing data, the status column sometimes is not present until actual data is present:
While it does not prevent the block to be read into a DataFrame (with four columns, in this case), the status column would start with
Noneinstead of any subscriptable string, causing aTypeError.The fix instead gets the first valid value to infer if the column is status or not. Further, it updates the method to infer if the first value is numeric by trying to convert it to float.
Question: If
not len(first_value) in [3, 5, 13, 17], should the code emit a warning/error to contact MNE developers?Moreover, #13550 mentioned a scenerio that a recording block could be completely empty. While it doesn't seem to trigger any error anymore, this is not explicitly tested in the current tests. So we will also add this to the test coverage.
Minor fixes
if "PUPIL" in units, warn "Raw pupil position data detected" instead of "Raw eyegaze coordinates detected".Additional information
The main focus of the PR is expanding
_simulate_eye_tracking_data(which is already simulating multi-block data) to simulate more edge cases.