Skip to content

Add read_raw wrapper function#4675

Closed
cbrnr wants to merge 1 commit intomne-tools:masterfrom
cbrnr:read_raw
Closed

Add read_raw wrapper function#4675
cbrnr wants to merge 1 commit intomne-tools:masterfrom
cbrnr:read_raw

Conversation

@cbrnr
Copy link
Copy Markdown
Contributor

@cbrnr cbrnr commented Oct 17, 2017

Would it be useful to have a wrapper read_raw function that calls specific readers based on the file extensions?

@christianbrodbeck
Copy link
Copy Markdown
Member

christianbrodbeck commented Oct 17, 2017 via email

@larsoner
Copy link
Copy Markdown
Member

See also discussion in #1930

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 17, 2017

I'd argue not to replace the existing read_raw_* functions with a generic read_raw function. In fact, read_raw should just be a wrapper to make it easier for users to load common data formats.

@christianbrodbeck read_raw only needs one argument (the file name), I'm passing everything else to the corresponding read_raw_* functions. So this shouldn't be an issue.

@jasmainak
Copy link
Copy Markdown
Member

@teonbrooks already implemented something similar in our mne-bids repo.

from .eeglab import read_raw_eeglab


def read_raw(input_fname, **kwargs):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then how do you find out what parameters to pass? I find it clearer to be explicit by using the proper dedicated function

@teonbrooks
Copy link
Copy Markdown
Member

I think that there may be some value in exploring a generic read_raw function, e.g. support for bids export as @jasmainak mentioned above, but I do think we should work out standardizing the standards across the different file types instead of the catchall **kwargs.
the problem with the kwargs is that you often need to reference the format specifics if all the formats aren't listed in the docstring and at that point, you could just use the existing function. I think if we did have it, we would want it to cover of ~80% use case, and then point to specific readers for add'l functionality.

just my 2c

@jasmainak
Copy link
Copy Markdown
Member

standardizing the standards

The irony! :)

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 18, 2017

Yes, I agree. If we just pass **kwargs the user has to read the doc of the specific function, so this doesn't really make things easier. I just thought it is kind of silly from a user's perspective to have multiple reader functions if the goal is to read an EEG/MEG raw file - most users won't care about the format, they just want to load the data. So yeah, either we start discussing a complete revamp of the io functions, or we'll just leave it as is for now.

@jona-sassenhagen
Copy link
Copy Markdown
Contributor

I think I agree with @teonbrooks - if the args of the different readers are 95% the same, so that a generic wrapper would have 95% of the docstring for the specific functions, we could probably have a tiny bit of **kwargs at the end.

We are, after all, doing this with montages already.

elif ext == '.data':
return read_raw_nicolet(input_fname, **kwargs)
elif ext == '.set':
return read_raw_eeglab(input_fname, **kwargs)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, I'd use a dict for this.

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Oct 22, 2017 via email

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 24, 2017

I agree, this is probably not such a great idea. The GUI will abstract this away anyway, so there's no need for a (private) helper.

@cbrnr cbrnr closed this Oct 24, 2017
@cbrnr cbrnr deleted the read_raw branch October 24, 2017 07:00
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.

7 participants