MRG FIX: Unifying raw reading

[larsoner]

Refactoring is only complete for RawFIF so far. I have moved _read_segment into _BaseRaw, and it calls _read_segment_file, which is a more targeted function. Now I need to transform the reading functions for KIT and EDF to work the same way. Hopefully we'll end up with more red lines than green ones.

Closes #2121.

[larsoner]

Alright, tests pass over here. Reading code has been unified quite a bit. Ready for review/merge from my end.

We might want to delay release for a few days so we can all test this and make sure everything is working okay...

[coveralls]

Coverage increased (+0.03%) to 90.37% when pulling 201c97d on Eric89GXL:fix-read into db84820 on mne-tools:master.

[coveralls]

Coverage decreased (-17.26%) to 73.09% when pulling 201c97d on Eric89GXL:fix-read into db84820 on mne-tools:master.

[dengemann]

And what if you need to pass config and hs_file?

[larsoner]

See comments in code, but that would go in _raw_extras, which is supposed to be a list the same length as the number of underlying raw files if it's used. You can make whatever you want as entries in the list, RawFIF stores one FIF tag directory per file, EDF stores a dict and I think KIT does, too

[dengemann]

Not sure I get it. Would this work if you passed additional params like config and headshape for BTI?

[larsoner]

Yeah, it would work. Right now BTI doesn't support non-preloaded data, so it's a moot point currently. But in the future when someone wants to do it, they can put whatever they want in _raw_extras, one per loaded raw file. That could include whatever params and files need to be parsed in order to construct data in _read_segment_file. Actually the EDF reader already does it by storing things like annot, see below where this happens:

annot = self._raw_extras[fi]['annot']

It pulls the annot param from the given file number.

[dengemann]

@Eric89GXL really 0.9?

[larsoner]

I'd slightly prefer 0.10 actually since it's a big change to get KIT and EDF working, but I think @agramfort wanted it. How about this:

• I open a PR for 0.9 that prevents EDF and KIT files from being concatenated, unless preload is on for all files, and the output is in mode preload=True
• Merge this PR into 0.10 after release, and remove that restriction

?

[larsoner]

@dengemann see @agramfort's comments in the related issue #2121 (comment)

[agramfort]

I'd slightly prefer 0.10 actually since it's a big change to get KIT and
EDF working, but I think @agramfort https://github.com/agramfort wanted
it. How about this:

• I open a PR for 0.9 that prevents EDF and KIT files from being
  concatenated, unless preload is on for all files, and the output is in
  mode preload=True
• Merge this PR into 0.10 after release, and remove that restriction

?

+1 I afraid of shipping something broken at the core level

[teonbrooks]

my understanding of mult in the info is that it is there but it is never used. this is the reason the data is being stored in V instead of uV. Also there is no corresponding unit for uV listed in the constants.

[larsoner]

mult is not in the info, at least not this incarnation of mult. If you look in _BaseRaw, you'll see that mult contains the compensation, projectors, plus cals, and it's constructed on the fly. But cals aren't used in these readers the same way they're used in the other readers. We can unify it at some point, but it would be easier just to "un-cal" the mult matrix and use it here if we want to support mult.

[teonbrooks]

ah, I was thinking this is unit_mul. it's what I was calling gain in EDF. got it

[teonbrooks]

what I meant is that I think it has the same concept of the gain that is being used.

[larsoner]

Okay I'll open another PR hopefully shortly. Then okay to push out release today?

[dengemann]

Okay I'll open another PR hopefully shortly. Then okay to push out release today?

yes.

[agramfort]

Okay I'll open another PR hopefully shortly. Then okay to push out release
today?

ok for me. Let's do the pypi push and update the website today in github.
When it's online at MGH we can announce it ok?

[larsoner]

Sounds good. I guess Martin set that up for 4 AM or so? Or can you manually do it again with the instructions he gave you? I can also ping Hari later today if you want.

[agramfort]

I can do it manually later. I am in a train...

[larsoner]

And you don't have SSH working on your phone? Is this amateur hour?

[dengemann]

@Eric89GXL alex does not update his devices because auf backwards compatibility concerns.

[agramfort]

I live in the stone age :)

[agramfort]

I am too old. I've learnt my lessons :)

[dengemann]

Imagine users' wouldn't update MNE-Python for the same reasons, ..., what would all our efforts be good for? :)

[larsoner]

Maybe @agramfort is still using 0.7 and just helps us test newer versions out of the goodness in his heart

[dengemann]

Maybe @agramfort is still using 0.7 and just helps us test newer versions out of the goodness in his heart

you mean 0.1 stable ?

[larsoner]

Alright, rebased. @dengemann @agramfort okay with you for merge? This one is a prereq for @alexandrebarachant in #2136, but if you think we should do additional testing it could wait a week or two. (I wonder if it's possible for big screwups to escape our testing at this point...)

[agramfort]

how much can we factorize the tests by add a raw object test function in test_raw.py and rely on this to test all classes? diff my get ugly so let's probably wait for another PR especially if tests gets deleted.

[agramfort]

I would also try to build the doc on this branch to see if everything looks ok.

I did not spot any obvious issue.

[larsoner]

ahh good call. I can do that Monday

[larsoner]

(the doc building I mean, I agree that the test refactoring should be a separate PR)

[larsoner]

Docs LGTM. Ready for final review/merge from my end.

[agramfort]

thanks @Eric89GXL !