allow BIDSPath(check=False) to work for _check_key_val()

obergmartin commented 8 months ago

Describe the problem

Would it be too much of an abuse of mne_bids.BIDSPath to allow "/" as a directory separator in the datatype field when check=False is passed as a parameter? I'm trying to organize my derivative files and would like to use datatype as a subdirectory field.

This is the current error:

File [~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:229](https://file+.vscode-resource.vscode-cdn.net/home/martin/code/eegpipeline/~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:229), in _check_key_val(key, val)
    [227](https://file+.vscode-resource.vscode-cdn.net/home/martin/code/eegpipeline/~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:227) """Perform checks on a value to make sure it adheres to the spec."""
    [228](https://file+.vscode-resource.vscode-cdn.net/home/martin/code/eegpipeline/~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:228) if any(ii in val for ii in ['-', '_', '[/](https://file+.vscode-resource.vscode-cdn.net/)']):
--> [229](https://file+.vscode-resource.vscode-cdn.net/home/martin/code/eegpipeline/~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:229)     raise ValueError("Unallowed `-`, `_`, or `/` found in key/value pair"
    [230](https://file+.vscode-resource.vscode-cdn.net/home/martin/code/eegpipeline/~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:230)                      f" {key}: {val}")
    [231](https://file+.vscode-resource.vscode-cdn.net/home/martin/code/eegpipeline/~/mambaforge/envs/mne/lib/python3.11/site-packages/mne_bids/utils.py:231) return key, val

ValueError: Unallowed `-`, `_`, or `/` found in key/value pair datatype: First/Second

Specifically, I'd like to nest preprocessing steps and their intermediate outputs in a directory heirarchy, eg: Rereference/ComputeICA/RejectICA/Epoch, but, for example, allow different rejected IC combinations in order to examine the differences in data quality between choices. Thus I would also have: ComputeICA/RejectICA2/Epoch as a directory hierarchy.

Describe your solution

can we change path.py line 994 (in BIDSPath.update) to something like: if key in ["suffix", "datatype"] and not self.check:

That allows this code to execute:

import mne
from mne_bids import BIDSPath
b_path = BIDSPath(subject="999", root="derivatives/pipeline-test/", description="test", datatype="First/Second", suffix="eeg", extension=".fif", check=False)
b_path

with the desired output: PosixPath('derivatives/pipeline-test/sub-999/First/Second/sub-999_desc-test_eeg.fif')

Describe possible alternatives

I know this is a bit of an abuse of the BIDSPath, but I do like having mne_bids do the ordering of all the other key-values pairs. I've read in the docs that BIDS derivative files can be non-compliant. It doesn't seem too much of a stretch to use datatype as a proxy for derivative preprocessing steps since is is a new "type" of data resulting from preprocessing. But I'm also interested in hearing other ideas or suggestions.

Additional context

I know this allows for more that my specific use case, but with the great power of check=False comes great responsibility!

welcome[bot] commented 8 months ago

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

obergmartin commented 7 months ago

After thinking through this for a while I have come to thinking that while this would be BIDS abuse and could be useful for organizing (non-BIDS compliant) derivatives, it isn't needed and BIDS should keep doing what it does and doing it well. Although I'm moving on, I would be interested if anyone reading in the future this has any thoughts.

sappelhoff commented 7 months ago

Hi @obergmartin sorry for the late response. I am glad you found a solution that is working for you.

For the record, I wouldn't have liked to code such a "trick" into BIDSPath, because the more such "use case tricks" we support, the less clear the overall functionality becomes. Furthermore, I think that using some additional lines of custom code can easily achieve what you had in mind (which is probably what you ended up with).

One note for future issues that you write. You wrote:

can we change path.py line 994 (in BIDSPath.update) to something like:

You can simply navigate to the file in the repo, find the line, and generate a permalink to it, see this screenshot:

grafik

Once you have done that, you can paste the link here and it renders nicely:

https://github.com/mne-tools/mne-bids/blob/c16d3b23a242822bc5d2a3e7f976323c9d487978/mne_bids/path.py#L994

This can be helpful for people reading your issue, as they can directly jump into the code :-)

mne-tools / mne-bids