mne-tools / mne-bids

MNE-BIDS is a Python package that allows you to read and write BIDS-compatible datasets with the help of MNE-Python.
https://mne.tools/mne-bids/
BSD 3-Clause "New" or "Revised" License
133 stars 88 forks source link

read_raw_bids can not infer 'task' even if BIDSPath can? #1014

Open kaare-mikkelsen opened 2 years ago

kaare-mikkelsen commented 2 years ago

Describe the bug

If a recording has a 'task' key, but the value is implicit, read_raw_bids is unable to read it. This is different from BIDSPath, which is able to infer it if there is only one possibility.

Steps to reproduce

code:

This will work:

bidsDir='E:\dataMoving\EESM1a_kaare\derivatives\cleaned_1'
filePath=mb.BIDSPath(root=bidsDir,subject='002',session='001',datatype='eeg',extension='.set',task='sleep')
print(filePath)
print(os.path.exists(filePath))
raw=mb.read_raw_bids(filePath)

Output:

E:/dataMoving/EESM1a_kaare/derivatives/cleaned_1/sub-002/ses-001/eeg/sub-002_ses-001_task-sleep_eeg.set
True

(I have excluded a lot of output from read_raw_bids())

This won't:

filePath=mb.BIDSPath(root=bidsDir,subject='002',session='001',datatype='eeg',extension='.set')
print(filePath)
print(os.path.exists(filePath))
raw=mb.read_raw_bids(filePath)

Output:

E:/dataMoving/EESM1a_kaare/derivatives/cleaned_1/sub-002/ses-001/eeg/sub-002_ses-001_task-sleep_eeg.set
True

---------------------------------------------------------------------------
FileNotFoundError                         Traceback (most recent call last)
Input In [82], in <cell line: 6>()
      4 print(filePath)
      5 print(os.path.exists(filePath))
----> 6 raw=mb.read_raw_bids(filePath)

File <decorator-gen-579>:12, in read_raw_bids(bids_path, extra_params, verbose)

File c:\Users\au207178\Anaconda3\envs\cuda11\lib\site-packages\mne_bids\read.py:683, in read_raw_bids(bids_path, extra_params, verbose)
    [680](file:///c%3A/Users/au207178/Anaconda3/envs/cuda11/lib/site-packages/mne_bids/read.py?line=679)             break
    [682](file:///c%3A/Users/au207178/Anaconda3/envs/cuda11/lib/site-packages/mne_bids/read.py?line=681) if not raw_path.exists():
--> [683](file:///c%3A/Users/au207178/Anaconda3/envs/cuda11/lib/site-packages/mne_bids/read.py?line=682)     raise FileNotFoundError(f'File does not exist: {raw_path}')
    [684](file:///c%3A/Users/au207178/Anaconda3/envs/cuda11/lib/site-packages/mne_bids/read.py?line=683) if config_path is not None and not config_path.exists():
    [685](file:///c%3A/Users/au207178/Anaconda3/envs/cuda11/lib/site-packages/mne_bids/read.py?line=684)     raise FileNotFoundError(f'config directory not found: {config_path}')

FileNotFoundError: File does not exist: E:\dataMoving\EESM1a_kaare\derivatives\cleaned_1\sub-002\ses-001\eeg\sub-002_ses-001_eeg.set

Expected results

If BIDSPath() can infer the task, that logic should be utilized by read_raw_bids() as well. Instead of reading the field values directly from the BIDSPath object, read_raw_bids() should use get_entities_from_fname(). Then whatever smart logic is hidden in BIDSpath can be reused by read_raw_bids(). When I call mb.get_entities_from_fname(str(filePath)), it retrieves all the correct information.

Actual results

If task is not explicitly set, read_raw_bids() fails.

welcome[bot] commented 2 years ago

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

agramfort commented 2 years ago

you suggest that it should guess the task key if only one task is present? even if you do not explicitly provide it?

Message ID: @.***>

kaare-mikkelsen commented 2 years ago

yes, like BIDSpath does. right now I am passing a BIDSpath object which prints as

E:/dataMoving/EESM1a_kaare/derivatives/cleaned_1/sub-002/ses-001/eeg/sub-002_ses-001_task-sleep_eeg.set

which is the file I have in mind. However, read_raw_bids will complain that this file does not exist:

E:\dataMoving\EESM1a_kaare\derivatives\cleaned_1\sub-002\ses-001\eeg\sub-002_ses-001_eeg.set

This is also correct, but irrelevant, since I was asking for the one with "task=sleep". BIDSPath knows this, so why does read_raw_bids not? Another solution would be to have BIDSPath be less intelligent, and simply complain that no such file exists, instead of guessing the task. But that seems a downgrade, when the solution is simple to implement?

hoechenberger commented 2 years ago

If BIDSPath infers the task then I believe we should classify this behavior (inferring the path) as a bug and drop this.

The task is a required BIDS entity and if not present, we should not try to be clever.

We've tried just that (being clever) many times in the past and it leads to breakage down the road, sometimes sooner, sometimes later.

hoechenberger commented 2 years ago

If BIDSPath infers the task then I believe we should classify this behavior (inferring the task) as a bug and drop this.

The task is a required BIDS entity and if not present, we should not try to be clever.

We've tried just that (being clever) many times in the past and it leads to breakage down the road, sometimes sooner, sometimes later.

agramfort commented 2 years ago

I am also inclined to consider that guess the task is black magic and likely to produce issues in many cases. I would just drop this option.

Message ID: @.***>

adam2392 commented 2 years ago

Another issue I just ran into with regards to BIDSPath trying to infer the full path is when extension is specified, but suffix is not. There is no error and the full path can't be written out.

I think in general, is there a BIDS spec on what MUST be specified in a path and what is optional? Then we can I think refactor the BIDSPath based on this and also write error messages to indicate what entities are missing from a user.

hoechenberger commented 2 years ago

what MUST be specified in a path and what is optional?

I only know it for MEG and EEG from the top of my head, but there the minimum you need is: