Skip to content

Conversation

@P-Oveisi
Copy link
Collaborator

@P-Oveisi P-Oveisi commented Aug 1, 2022

The fetch_file() function only downloads the file, and then the file has to be loaded in manually. If the fetched file is a snirf, and if the load_raw argument is true, load in the downloaded file as an mne raw object.

I'm not sure if it's best to keep the downloading and the loading parts separate or to integrate them.

(also, corrected the 'hmb' filetypes to 'hbm' in the info.txt file.)

@P-Oveisi P-Oveisi requested a review from JohnGriffiths August 1, 2022 20:51
@JohnGriffiths
Copy link
Contributor

I was requested to review this but there is no description in the PR comments of what it's doing and why.

The `fetch_file()` function would download a file from the links in the info.txt file, and to load the snirfs in there needed to be an additional step using `read_raw_snirf()`. Now if `load_raw` argument is true, the function will return a dictionary containing the loaded raw file(s) so it doesn't have to be done manually.
If the fetched file is a snirf, and if the `load_raw` argument is true, load in the downloaded file as an mne raw object.
@P-Oveisi
Copy link
Collaborator Author

P-Oveisi commented Aug 2, 2022

I was requested to review this but there is no description in the PR comments of what it's doing and why.

@JohnGriffiths updated with description

@JohnGriffiths
Copy link
Contributor

I'm not sure I agree with the approach of folding data loading into a data fetching function. They are different operations.

What is the advantage of this change exactly?

If / when we add major changes for data fetchers, we should probably just follow the nilearn approach

https://nilearn.github.io/dev/modules/datasets.html

https://github.com/nilearn/nilearn/blob/main/nilearn/datasets/atlas.py

https://nilearn.github.io/dev/manipulating_images/input_output.html#datasets

which usually return Bunch objects with lots of info, which for things like nifti files is a text string file path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants