Skip to content

WIP add a general read_raw function#1930

Closed
teonbrooks wants to merge 13 commits intomne-tools:masterfrom
teonbrooks:read_raw
Closed

WIP add a general read_raw function#1930
teonbrooks wants to merge 13 commits intomne-tools:masterfrom
teonbrooks:read_raw

Conversation

@teonbrooks
Copy link
Copy Markdown
Member

The proposal is to have one read_raw function that will read any
supported data format and internally point it to the proper reader module.
It uses the default settings of those readers except for the exposed
parameters.
This will rely on the mne.define_sensor #1909 (currently referred to as
mne.rename_sensor in PR).

@teonbrooks teonbrooks force-pushed the read_raw branch 2 times, most recently from ae26fb9 to b25439b Compare April 6, 2015 13:37
@agramfort
Copy link
Copy Markdown
Member

I like the idea to have a single read_raw function.

I'll everybody comment on the DigMontage / Montage thing.

@teonbrooks
Copy link
Copy Markdown
Member Author

should I split this pr into two? i guess technically they are two separate
features

-teon

On Mon, Apr 6, 2015 at 6:04 PM, Alexandre Gramfort <notifications@github.com

wrote:

I like the idea to have a single read_raw function.

I'll everybody comment on the DigMontage / Montage thing.


Reply to this email directly or view it on GitHub
#1930 (comment).

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 6, 2015 via email

@teonbrooks teonbrooks changed the title WIP add a general read_raw function, DigMontage WIP add a general read_raw function Apr 6, 2015
mne/io/base.py Outdated
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.

we will need something like a params argument to pass additional parameters. Depending on the paths of the user this call might not work (config, hs_file).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

does BTi data have an extension? I was guessing at one. I looked through the module and it didn't seem like the test data had one. if not, I guess since it is the only one without an extension, if op.splitext doesn't return anything, we could assume that it is this format.

yeah, I was trying to figure out the best way that case with BTi with those parameters. so for a params arg, you propose something like a dict for the add'l ones?

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.

@teonlamont they don't have extensions ....

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.

The pattern you can look for is commas in the file name following a 'c'

@christianbrodbeck
Copy link
Copy Markdown
Member

Isn't a problem for this that different raw types have different file requirements? For example for KIT you'd want arguments for head_shape, ELP and marker files (https://github.com/mne-tools/mne-python/blob/master/mne/io/kit/kit.py#L619). Or is the idea for this to be a quick-and-dirty reader that can't specify those additional parameters?

@christianbrodbeck
Copy link
Copy Markdown
Member

christianbrodbeck commented Apr 6, 2015

Or is the idea to supply a universal DigMontage object from your other PR?

@mainakjas
Copy link
Copy Markdown
Contributor

I like the idea of a single read_raw function too :)

@teonbrooks
Copy link
Copy Markdown
Member Author

@christianbrodbeck the idea would be to have a universal DigMontage.

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 6, 2015 via email

@teonbrooks
Copy link
Copy Markdown
Member Author

it won't be quick and dirty. if it is to be done, it will be done right :)
I think it would serve a good purpose too. the idea I have is to have a standard api for all systems so entry to the software is simple and straightforward. the workflow I have in mind would be something like this for every system:

EEG:
read_montage (electrode positions either template or customized) --> read_raw --> happiness

MEG:
read_dig_montage (digitizer head info) --> read_raw --> happiness

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 6, 2015 via email

@christianbrodbeck
Copy link
Copy Markdown
Member

  • Since MEG/EEG and digitizer acquisition systems are usually independent AFAIK I like the idea in general of separating digitizer and MEG/EEG data handling, so you can combine appropriate digitizer and data handling in a modular fashion.
  • It doesn't make much of a difference if I call read_raw or read_raw_kit (I'm not switching MEG systems daily :) ) but if it's possible without loss of functionality why not
  • would the returned objects all be completely interchangeable for different data types? I thought there were some things you could not do with all Raw objects at some point?

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 6, 2015 via email

@teonbrooks
Copy link
Copy Markdown
Member Author

Since MEG/EEG and digitizer acquisition systems are usually independent AFAIK I like the idea in general of separating digitizer and MEG/EEG data handling, so you can combine appropriate digitizer and data handling in a modular fashion.

I agree. I like this too :) i've also been reading a lot of Fodor recently ;)

It doesn't make much of a difference if I call read_raw or read_raw_kit (I'm not switching MEG systems daily :) ) but if it's possible without loss of functionality why not

hehe yeah. one use case is if you have both EEG and MEG and they are not automagically combined like in the neuromag set up, the api for creating the raw objects will now be the same for both data formats.
You could do it the current way with the marked reader, but it makes for a easy transition if you're coming from another software package, like fieldtrip, where you can read any data format with one function ft_preprocessing.

would the returned objects all be completely interchangeable for different data types? I thought there were some things you could not do with all Raw objects at some point?

it is already implemented that the returned raw object will have all the same attributes and methods based on the _BaseRaw. they will have a private system-specific dict that contains info from their header files. the application of any montage will work the same across the systems too.

@christianbrodbeck
Copy link
Copy Markdown
Member

Sounds good

mne/io/base.py Outdated
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.

BioSemi systems use the '.bdf' extension, so

elif ext == '.edf' or ext == '.bdf'

@teonbrooks teonbrooks added this to the 0.9 milestone Apr 10, 2015
@teonbrooks teonbrooks force-pushed the read_raw branch 3 times, most recently from f5578f7 to 2116c55 Compare April 14, 2015 17:44
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.

For RawFIFF you need to have the allow_maxshield option. Or should we have people use read_raw_fif if they need more options? If that's the case, add a Notes section making it clear that not all options are exposed and users might need to use type-specific readers. Also add See also section with the relevant readers.

@larsoner
Copy link
Copy Markdown
Member

What else needs to be done here @teonlamont?

@larsoner
Copy link
Copy Markdown
Member

If nothing, then we can merge once you 1) add the comment I mention above, 2) update whats_new and python_reference, and 3) swap in this function at least once for each of the use cases it covers in our tests.

@teonbrooks
Copy link
Copy Markdown
Member Author

should the function be mne.read_raw? I currently have it being import as mne.io.read_raw but I think it would make more sense to have the first option.

@teonbrooks
Copy link
Copy Markdown
Member Author

@dengemann check out 7bf1620 for the implementation of a read_params; this is what I initially had.

since config is actually str and it is the default, we technically don't need this additional parameter to capture other settings. I think it might be fine how it was. I'm +1 for leaving the function without **kwargs or reader_params.

@larsoner
Copy link
Copy Markdown
Member

@dengemann you can't introspect reader_params either. I find this:

read_raw('fname_raw.fif', preload=True, allow_maxshield=True)

Better than this:

read_raw('fname_raw.fif', preload=True, reader_params=dict(allow_maxshield=True))

It is forcing another param to act as kwargs without really providing any benefit.

@larsoner
Copy link
Copy Markdown
Member

and read_raw(..., reader_params=dict(fif=dict(allow_maxshield=True))) is even worse...

@dengemann
Copy link
Copy Markdown
Member

At least this is a pattern we already support. Kwargs is not. I would really like to avoid kwargs. The other thing is what Alex mentioned: more ways to do the same thing. I'm really not sure what to do with this. Mixed feelings.

On 19 Apr 2015, at 04:48, Eric Larson notifications@github.com wrote:

and read_raw(..., reader_params=dict(fif=dict(allow_maxshield=True))) is even worse...


Reply to this email directly or view it on GitHub.

@larsoner
Copy link
Copy Markdown
Member

FWIW, we do have kwargs at least one other place, in raw.apply_function. One prominent use kwargs (and even what we use it for currently) is passing keyword arguments to a function that will call another function, whose appropriate keyword arguments can vary. That is essentially what is being done here. It's what e.g. scipy and numpy do in these situations AFAIK. The other proposed solutions do not seem as simple, elegant, or useful to me. Why do people dislike kwargs for this type of use case?

Now, whether or not we should have read_raw at all, I'm +0 on, since I'm happy to just use read_raw_fif anyway.

@dengemann
Copy link
Copy Markdown
Member

Because I hated it always in matplotlib and elsewhere, it's too intransparent, especially if you cannot look it up in the docs. So if we support it we should at least make all accepted parameters explicit in the doc string... Or just stick with what we have as io functions...

On 19 Apr 2015, at 05:13, Eric Larson notifications@github.com wrote:

FWIW, we do have kwargs at least one other place, in raw.apply_function. One prominent use kwargs (and even what we use it for currently) is passing keyword arguments to a function that will call another function, whose appropriate keyword arguments can vary. That is essentially what is being done here. It's what e.g. scipy and numpy do in these situations AFAIK. The other proposed solutions do not seem as simple, elegant, or useful to me. Why do people dislike kwargs for this type of use case?

Now, whether or not we should have read_raw at all, I'm +0 on, since I'm happy to just use read_raw_fif anyway.


Reply to this email directly or view it on GitHub.

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.

Our pattern is not to use assert statements at all, but to do a proper check and throw an error, should change for consistency. Plus it should probably be ValueError, this will end up being an AssertionError

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.

+1

@larsoner
Copy link
Copy Markdown
Member

@teonlamont did you take care of #1966 here? If so, please add Closes #1966 to the description.

@larsoner
Copy link
Copy Markdown
Member

@dengemann but what you proposed with reader_params is no more explicit... I don't get it.

@teonbrooks teonbrooks mentioned this pull request Apr 19, 2015
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.

don't use nose outside of tests. It makes nosetests a strong dependency of MNE

@agramfort
Copy link
Copy Markdown
Member

I like the idea but this needs some thinking as it's really core of the library

@larsoner
Copy link
Copy Markdown
Member

@agramfort have you done some thinking? :)

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 22, 2015 via email

@larsoner larsoner modified the milestones: 0.10, 0.9 Apr 22, 2015
@dengemann
Copy link
Copy Markdown
Member

+1

2015-04-22 21:58 GMT+02:00 Alexandre Gramfort notifications@github.com:

I would vote for 0.10 as it's a big change. Let's play with it for a few
weeks before adding this to a release.


Reply to this email directly or view it on GitHub
#1930 (comment).

@agramfort
Copy link
Copy Markdown
Member

I am closing as I am know thinking we will have to keep the other functions and we want only one way to do things

@agramfort agramfort closed this Aug 15, 2015
@larsoner
Copy link
Copy Markdown
Member

I am currently working on trying to get FIF integration with scitran data management systems. I'm starting with only FIF support, but in theory mne-python could serve as a backend for all the M/EEG datatypes it supports. This would be facilitated quite a bit by having a single, auto-triaging read_ function along the lines of nibabel.load(...). So there is at least one decent use case. But we can deal with that later. I need to get FIF working first. :)

@agramfort
Copy link
Copy Markdown
Member

I need to get FIF working first. :)

+1 :)

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