mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Add buttons

Open WouterKroot opened this issue 6 months ago • 1 comments

Reference issue (if any)

What does this implement/fix?

Additional information

WouterKroot avatar Jun 16 '25 14:06 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!

WouterKroot avatar Jun 17 '25 14:06 WouterKroot

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

scott-huberty avatar Aug 07 '25 13:08 scott-huberty

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

WouterKroot avatar Aug 11 '25 14:08 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.

WouterKroot avatar Aug 12 '25 15:08 WouterKroot

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

https://github.com/mne-tools/mne-python/blob/b64c8231febe93926ec798c01ff01b5cb78055c3/mne/io/eyelink/tests/test_eyelink.py#L400

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.

scott-huberty avatar Aug 12 '25 17:08 scott-huberty

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

WouterKroot avatar Aug 12 '25 18:08 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

WouterKroot avatar Aug 13 '25 07:08 WouterKroot

@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 avatar Aug 13 '25 08:08 cbrnr

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

cbrnr avatar Aug 13 '25 08:08 cbrnr

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.

scott-huberty avatar Aug 13 '25 20:08 scott-huberty

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.

WouterKroot avatar Aug 14 '25 09:08 WouterKroot

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.

cbrnr avatar Aug 14 '25 09:08 cbrnr