Add buttons

[WouterKroot]

Reference issue (if any)

What does this implement/fix?

Currently, the parsing of button press events from eyelink edf files is not supported.
With minor changes to io/eyelink/_utils.py and the the associated new tests, such as the creation of datafiles, all eyelink data can be parsed.

Additional information

[larsoner]

Looks reasonable to me! Just a few minor comments

[larsoner]

Looks like reasonable modifications, but later I'd expect some assertions about the raw.annotations or something for example that the right number of button events occurred etc.

[larsoner]

@WouterKroot do you need more guidance on this part specifically?

[WouterKroot]

Yes please, not sure that I follow. What do you mean by assertions, as in tests?

[larsoner]

So if you look later in the file you see stuff like

mne-python/mne/io/eyelink/tests/test_eyelink.py

Lines 318 to 319 in b50a29a

	assert raw.annotations.description[1] == "SYNCTIME"
	assert raw.annotations.description[-1] == "BAD_ACQ_SKIP"

I would expect similar assert statements to work here. So some of the annotations now should have some button description, there should be a specific number that have a button description, etc. So something like the following (untested, needs to be adapted)

button_idx = [ii for ii, desc enumerate(raw.annotations.description) if "button" in desc.lower()]
assert len(button_idx) == 6  # or however many you added
assert_allclose(raw.annotations.onset[button_idx[0]], 1.2356, atol=1e-5)  # or whatever the onset of the first simulated button press was

make sense?

And you can add to the test and test locally with

pytest mne/io/eyelink -k multi_block_misc

to make sure it passes before pushing

[WouterKroot]

Thanks for the comments, to be fair I don't really understand the eyelink asc format. Also I am new to these tests and how to efficiently push relevant changes. Please let me know if you have more suggestions and comments!

[scott-huberty]

Getting there! that simulate_data function was already kind of hairy, so nice start @WouterKroot.

the test_multi_block_misc_channels test was failing on your branch, so the changes I suggested should get things passing..

But you will need find these lines:

assert raw.annotations.description[-1] == "BAD_ACQ_SKIP"
assert np.isclose(raw.annotations.onset[-1], 1.001)
assert np.isclose(raw.annotations.duration[-1], 0.1)

And change them to something like

assert raw.annotations.description[-1] == "button_1_release"
# there should be 1 BAD_ACQ_SKIP annotation between our two blocks
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)

This is because the last annotation in this file used to be BAD_ACQ_SKIP, but now the last 6 or so annotations are the button events you've added, so our assert statements need to reflect that.

The last sticking point for me is that with this test case, I would expect the first button related annotation tone button_1_pressed but it appears to be button_1_released. Why is that?

>>> raw.annotations.description
array(['fixation', 'SYNCTIME', 'saccade', 'fixation', 'saccade',
       'fixation', 'saccade', 'fixation', 'saccade', 'BAD_ACQ_SKIP',
       'button_1_release', 'button_1_press', 'button_1_release',
       'button_1_press', 'button_1_release', 'button_1_press',
       'button_1_release'], dtype='<U16')
```

[scott-huberty]

Suggested change

	for timestamp in np.arange(5488500, 5488600): # 100 samples
	# And let's add some buttons events to this block
	for timestamp in np.arange(7453390, 7453490): # 100 more samples

So these new lines you appended to the end of the simulated file, actually had earlier timestamps than the previous lines, which breaks things. So I'm just making sure the timestamps are increasing.

[scott-huberty]

Suggested change

	elif event_type in ("EFIX", "ESACC", "BUTTON"):
	elif event_type in ("EFIX", "ESACC"):

[scott-huberty]

The while statement works, but you don't really need a loop here, you can just use a conditional statement:

Suggested change

	while (
	button_idx < len(button_events)
	and button_events[button_idx][0] == timestamp
	):
	if button_idx < len(button_events) and button_events[button_idx][0] == timestamp:

[scott-huberty]

Hey @WouterKroot do you still have time to finish this? I think that #12847 is going to touch a lot of code in the eyelink sub package, so it will make your life easier to merge this first! Let me know if you need any help

[WouterKroot]

Hey @scott-huberty, I'll work on it this week. Let you know if I need help, thanks!

[WouterKroot]

@scott-huberty, Hey Scott, I have been trying to get the button events to parse into the

def _simulate_eye_tracking_data(in_file, out_file), in test_eyelink.py

However, I'm having trouble with the "BAD_ACQ_SKIP" label. When I try and change the timestamps of the button I get:

======================================================================================== short test summary info ========================================================================================
FAILED mne/io/eyelink/tests/test_eyelink.py::test_multi_block_misc_channels[fname0] - AssertionError: assert np.str_('button_1_release') == 'BAD_ACQ_SKIP'
=================================================================================== 1 failed, 13 deselected in 0.71s =====

And changing the timestamps to be within the first block of simulated data did not work. Any suggestions on how to best parse in the button events? If it's unclear let me know.

[scott-huberty]

Ah ok! I think the failing unit test is this one:

mne-python/mne/io/eyelink/tests/test_eyelink.py

Line 400 in b64c823

assert raw.annotations.description[-1] == "BAD_ACQ_SKIP"

In that test, I originally asserted that the last annotation in the simulated raw file should be "BAD_ACQ_SKIP". That happened to be true at the time.

However, now that you’ve added button events, those button annotations might come after "BAD_ACQ_SKIP". Could you:

1. Inspect the raw object to see if the last annotation(s) are now button annotations.
2. Then, assuming point 1. is true, can you confirm that "BAD_ACQ_SKIP" is still the last annotation before the button annotations.

If so, we can adjust the test (there’s no reason BAD_ACQ_SKIP must be the final annotation).

From the root mne-python repo, You can run:

pytest --pdb mne/io/eyelink/tests/test_eyelink -k test_multi_block_misc_channels

When the test fails, you’ll drop into the pdb debugger. Then for example you can do something like raw.annotations.description[-10:] to inspect the last 10 annotations of that object.

[WouterKroot]

Okay makes sense, and point 1 and 2 are indeed the case. I’ll try and fix tomorrow

[WouterKroot]

@scott-huberty , All the button events work and test_multi_block_misc_channels also works. Tried to rebase and clean up the mess, but im still a new to git.

Problem now is that test_href_eye_events does not work, I tried changing the EYELINK_COLS in utils, I don't understand where these columns come from really. I tried to debug and found that:

Key: fixations, df columns: 10, expected: 7

Which gives the error. But changing the col_names does not help. See error below: any ideas?

Key: fixations, df columns: 10, expected: 7
F
mne/io/eyelink/tests/test_eyelink.py:527: in test_href_eye_events
E ValueError: Length mismatch: Expected axis has 10 elements, new values have 7 elements

[cbrnr]

@WouterKroot would it be possible to add a short description in your initial post about what this PR implements? This would make it easier for potential reviewers (and our PR template provides these fields, so don't just ignore them).

[cbrnr]

It would also be great if you rebased so that previous unrelated changes do not clutter this PR.

[scott-huberty]

Hey @WouterKroot sorry that you are having trouble with rebasing. If you want, I can meet with you 1:1 so that we can do this together. I will be at the MNE-Python Office hours this Friday at 15:00 UTC, held on MNE's discord server. If you can attend, we can break off and work through the rebase. If that doesn't work for you - feel free to send me an email and we will figure out a time that works.

[WouterKroot]

MNE-Python Office hours this Friday at 15:00 UTC, held on MNE's discord server.

@scott-huberty Thanks again for reaching out. I would surely appreciate your time for a little 1:1 session. I think the rebase turned out fine and the buttons work fine in the simulation and the assertion tests. I think there are just some weird things happening in the:

def _infer_col_names_for_block(block: dict) -> tuple[dict[str, list], list]:

and some strange indexing in:

def test_href_eye_events(tmp_path):

This gives me that error that:

mne/io/eyelink/tests/test_eyelink.py:527: in test_href_eye_events
E ValueError: Length mismatch: Expected axis has 10 elements, new values have 7 elements

I would really appreciate it if you would be able to do some debugging with me, unfortunately 15:00 UTC is a late for me. I would be able to do it earlier? I would also be available today, have to log off around 14:30 UTC.

[cbrnr]

I think the rebase turned out fine and the buttons work fine in the simulation and the assertion tests.

Did you already push? The "Files changed" tab at the very top still shows many unrelated changes.