ENH: Extend dataset fetcher for the Antisaccade task #21
Conversation
scott-huberty
changed the title
|
Looks good to me. Thanks, @scott-huberty, for adding this. We should probably start thinking about upping our game in terms of continuous integration. Maybe adding a test that runs this function would be a first step? If you'd rather not add this to this PR, I'll create a separate issue about adding CI to this project and some initial testing. |
|
Good idea @christian-oreilly - bc actually 888ffb8 would have added a regression... I'm re-requesting a review based on the new code I've added since 05df081 |
| pathlib.Path | ||
| Path to the downloaded file. | ||
| """ | ||
| task = _get_task_from_subject_id(subject) |
There was a problem hiding this comment.
Since get_subject_runs API now has a parameter task="DOT". We need to actually pass task="AS" in cases where the filename is an anti-saccade file, e.g. fetch_eegeyenet(subject="BZ2").
I created little helper function for this. For some reason it feels a little hacky, but it works.
So I just want to make sure everyone agrees with this approach.
There was a problem hiding this comment.
Seems fine to me. I think what is not optimal is for the task to be part of the subject ID (you cannot have the same subject doing two tasks), but that was decided by EEGEyeNet, so it is fine to use it I think...
| archive_name=f"{subject}_DOTS{run}_EEG.mat", | ||
| folder_name=f"EEGEYENET-Data/dots/{subject}", | ||
| archive_name=f"{subject}_{task}{run}_EEG.mat", | ||
| folder_name=f"EEGEYENET-Data/{task}/{subject}", |
There was a problem hiding this comment.
Should it be f"EEGEYENET-Data/{task.lower()}/{subject}" to be consistent with the previous code, or the case doesn't matter here?
There was a problem hiding this comment.
Oh thanks for bringing that up. It does matter, but I'm wondering if it is worth the change to keep the case consistent across the API. i.e., since we use "DOTS" and "AS" in the code, maybe we be consistent and name the folder "DOTS / "AS"?
The next time you download the data you need to be aware of this (so that you don't keep a copy in both "dots" and "DOTS" directories). WDYT? worth the change or no?
There was a problem hiding this comment.
Yes, fine with me. I was just not sure if that path was use for reading an existing repository (in which case it would crash) or to write up the data (which would be fine). From what you say, it seems to be the later.
There was a problem hiding this comment.
Yes good point - I think it is fine since we typically do..
fpath = eoglearn.datasets.fetch_eegeyenet()
raw = eoglearn.io.read_raw_eegeyenet(fpath)
Which should handle the change from "dots" to "DOTS" for us.
There might be somewhere in paper_2024 that is affected (if we hardcoded "EEGEYENET/data/dots" somewhere, but from a quick git grep "dots", it doesn't seem like this is the case.
| pathlib.Path | ||
| Path to the downloaded file. | ||
| """ | ||
| task = _get_task_from_subject_id(subject) |
There was a problem hiding this comment.
Seems fine to me. I think what is not optimal is for the task to be part of the subject ID (you cannot have the same subject doing two tasks), but that was decided by EEGEyeNet, so it is fine to use it I think...
2 participants
This PR extends our dataset fetcher to download the EEG files for other task collected by EEGEYENET, the anti-saccade task.
This task contains some 300 additional EEG-ET recordings.
if acceptable, I'll create a reader for the anti saccade data in a separate PR
EDIT: The list of files are extracted from here
This dataset is organized slightly different than the dots task. I can't tell yet if there are 300 subjects with 1 run each... or perhaps ~50 subjects with about 6 runs each. I've defaulted to the former, but if it turns out that the latter is true I will update this csv file.