ENH: Add button events to Eyelink #13379
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thanks @WouterKroot ! Per your messages in other threads (ref #13287 #13253 ), I really appreciate the time you've put into this and that you have other competing priorities that limit the amount of time you can contribute to MNE. That being said, I think this is really close. My comments mostly pertain to house keeping.
Since you've initiated this new PR, we don't have to worry about the rebase issues in #13287. Lastly, can you edit your message at the top of this thread, and add "fixes #13250"?
EDIT: Oh and one more thing. @WouterKroot please add a change log to mne-python/doc/changes/dev/13379.newfeature.rst. See the other changelog entries in the dev sub directory to get an idea of what to write. And go ahead and add your name to mne-python/doc/changes/dev/changes.inc if you are not already in there. Refer to the MNE-Python contributing guide for more details
| descriptions = "BAD_" + descriptions | ||
|
|
||
| ch_names = df["eye"].map(eye_ch_map).tolist() | ||
| # breakpoint() # debug |
There was a problem hiding this comment.
| # breakpoint() # debug |
|
|
||
| def get_button_description(row): | ||
| button_id = int(row["button_id"]) | ||
| action = "press" if row["button_pressed"] == 1 else "release" | ||
| return f"button_{button_id}_{action}" | ||
|
|
There was a problem hiding this comment.
MNE-Python usually prefers to use private functions (placed at the module level) over inner functions (ref style guide). Can you prepend this function name with an underscore and move it out of the _make_eyelink_annots function?
There was a problem hiding this comment.
I assume that this was something you used during development, but we don't need it in our codebase. Can you remove this file from this PR?
| # print(f"\nLine {li}: {line}") | ||
| # print(f"Tokens ({len(tokens)}): {tokens}") |
There was a problem hiding this comment.
| # print(f"\nLine {li}: {line}") | |
| # print(f"Tokens ({len(tokens)}): {tokens}") |
| assert np.isclose(raw.annotations.onset[-1], 1.001) | ||
| assert np.isclose(raw.annotations.duration[-1], 0.1) | ||
|
|
||
| print("\n=== Annotations ===") |
There was a problem hiding this comment.
| print("\n=== Annotations ===") |
| print(f"{onset:.3f}s dur={duration:.3f}s {desc}") | ||
|
|
||
| print("\n=== Recording block markers ===") |
There was a problem hiding this comment.
| print(f"{onset:.3f}s dur={duration:.3f}s {desc}") | |
| print("\n=== Recording block markers ===") |
| if desc == "BAD_ACQ_SKIP": | ||
| print(f"BAD_ACQ_SKIP at {onset:.3f}s") | ||
|
|
There was a problem hiding this comment.
| if desc == "BAD_ACQ_SKIP": | |
| print(f"BAD_ACQ_SKIP at {onset:.3f}s") |
Just FYI, MNE has its own logger function for sending output to the console, but more to the point, in MNE we don't normally print out information from tests.
| elif (key == "buttons") and (key in descs): | ||
| required_cols = {"time", "button_id", "button_pressed"} | ||
| if not required_cols.issubset(df.columns): | ||
| raise ValueError(f"Missing column: {required_cols - set(df.columns)}") |
There was a problem hiding this comment.
This if condition is not covered by a test. I'm wondering, is there some plausible scenario where we would expect these column names ('time', 'button_id', 'button_pressed') to be missing from the buttons dataframe? It looks like you hard code these column names during button dataframe creation, so I would expect that they will always be present? If so, I think this check is unnecessary.
| else: | ||
| logger.info("No button events found in this file.") |
There was a problem hiding this comment.
| else: | |
| logger.info("No button events found in this file.") |
So this else clause is happening at the acquisition block level. I noticed with my files that have multiple acquisition blocks, "No button events found in this file" gets printed to my console multiple times. But it should only be printed once.
I think that we have a few options.
- use the DEBUG level (
logger.debug) - Omit this logger message entirely.
- move this check to
_validate_dataor something like that so it only gets printed to the console once per file.
|
@WouterKroot are you planning to work on this one again? Or would it help if we (probably @scott-huberty) pushed some commits? |
|
@scott-huberty no reply here, do you have time to push some commits to get this one in? |
Sorry about that. I have been a bit swamped and don’t see myself having time for this soon. Hopefully you guys will manage and I would love to do some work in the future. |
|
No problem @WouterKroot it happens, thanks for letting us know! @scott-huberty you up for taking over then? |
Yep I will do this before the 1.11 deadline |
|
@scott-huberty we'll try to release mid-late next week, do you have time to look before then? |
Realistically Monday but yes should be able to do it. I think that this is really close to the finish line. |
3 participants
Reference issue (if any)
What does this implement/fix?
To enable the parsing of button presses, I changed the eyelink utilities and tests. Mainly, added button events to the eyelink data simulation, some tests to see if they are correctly parsed, and make sure that associated timing is parsed.
Additional information
My messy git prevented me to rebase, so was forced to create a new request.