Read mff evoked #3

ephathaway · 2020-09-14T23:35:56Z

This PR is the pre-PR so that we can review the code amongst BEL engineers before submitting the actual PR to mne-tools.

In this PR, we begin to address mne-tools#8038 by implementing reading of averaged MFF files. We add a function mne.io.read_evokeds_mff which takes in an averaged MFF file and returns an EvokedArray object with the data for each condition specified. Reading of signal data and some metadata is done through mffpy, which is made a dependency of mne.

There are a few things we will need to sort out before this is ready to merge that I will list right off the bat.

mne testing datasets are stored in a separate repo. I am not sure what their process is for adding new test datasets, so for now I include my test averaged MFF file in this repository.
Right now there is no check to make sure the input MFF file is averaged. This will be important for preventing users from trying to read a segmented file with the mne.io.read_evokeds_mff function. I suggest we use the information in history.xml, which contains info on all of the tools run on the MFF file. We currently don't have a way to parse history.xml, so I suggest we first implement this capability in mffpy and then use it to read the info in mne.
The number of segments comprising each average is specified in categories.xml in an averaged MFF file, but again, we do not currently have the ability to parse this info. I suggest we implement this capability in mffpy as well.

ianbrown9475

Nice work, I ran the tests and it looks like the ones you added/modified are passing but there are others that aren't. I think some of them were already failing in the upstream code though. Could you look at fixing the CI action that's failing and fixing the linter errors? Looks like there's some more information on all of this here.

mne/io/egi/egimff.py

ephathaway · 2020-09-18T00:09:19Z

CIs appear to be passing now. I ran flake8 and docstyle tests and those look good. Some tests failing on my end as well, but they all look unrelated to my additions. Looks like they have to do with missing test files, so I might just be missing some tests datasets that I was supposed to install.

jusjusjus

Hi Evan, well done. You definitely hit the mark wrt their code base, but I would like this commit to be a little different here an there.

when including mffpy as a dependency, it would have been awesome if cached-property-bel automatically is added as a sub-dependency. Let's hear what they say about this, but get ready to include that into the pip wheel. We should also edit the "server_environment.yml"
The test data you added is good, and it's great that you check that mffpy actually reads it. I think we might need to clean up some of the viewer configuration files though, which are unnecessary for mffpy, e.g. "lastSettings.xml", "recordingSettings.xml". I don't think we support reading these.
You rightly named the child read_evokeds_..., b/c it returns mutliple. Instead of messing around with a list or an instance I would just return a list with one element. The branch (if else) will cause downstream branches by the guy that uses the API, otherwise.
It's good that you check the function input which immediately enhances the usability of the API. If a user inputs a wrong file than it'll come around and tell him. It's good to make these error messages very precise; tell the user what he did wrong. Also, throw a ValueError instead of an AssertionError (https://docs.python.org/3/library/exceptions.html#ValueError).
That's a matter of taste, but in general i'm not a big fan of abbreviations. If you want to avoid long names, then, one might be able to use context. One thing that would help in creating this context are functions. I think one could split _read_evoked_mff into 3-4 sub-functions that a called in sequence. In essence you do this, and you even write little documentation strings for each function, but by not actually creating a function your code always drags the whole context with it.

requirements.txt

mne/io/egi/tests/test_egi.py

mne/io/egi/egimff.py

ephathaway · 2020-09-22T18:39:09Z

You rightly named the child read_evokeds_..., b/c it returns mutliple. Instead of messing around with a list or an instance I would just return a list with one element. The branch (if else) will cause downstream branches by the guy that uses the API, otherwise.

I decided to return a single Evoked_Array if just reading one dataset instead of a list to stay consistent with mne.read_evokeds (for reading evoked .fif files).

jusjusjus

Thanks for addressing my comments. Let's see what they say about this.

ianbrown9475

Looks good to me!

ephathaway · 2020-09-24T18:44:54Z

Right now there is no check to make sure the input MFF file is averaged. This will be important for preventing users from trying to read a segmented file with the mne.io.read_evokeds_mff function. I suggest we use the information in history.xml, which contains info on all of the tools run on the MFF file. We currently don't have a way to parse history.xml, so I suggest we first implement this capability in mffpy and then use it to read the info in mne.

The number of segments comprising each average is specified in categories.xml in an averaged MFF file, but again, we do not currently have the ability to parse this info. I suggest we implement this capability in mffpy as well.

@ianbrown9475 @jusjusjus thank you both for reviewing this! Your feedback was very helpful. Before I submit a PR to mne-tools, I would like to address the above issues. I will do some work on mffpy next week and hopefully have PR submitted by Friday.

jusjusjus · 2020-09-25T11:00:57Z

Hey Evan, I think you would be right to check if the file is averaged beforehand.

jusjusjus

Looks good, great job.

ianbrown9475

Nice, looks good

jusjusjus

Looks good

ephathaway · 2020-11-09T17:43:38Z

Looks good

Thanks. I still think we should merge this into mne-tools/mne-python master branch when it's approved and then pull those changes into our master.

ianbrown9475 · 2020-11-09T17:45:42Z

@ephathaway Yeah I think that's pretty common for these kind of situations. Are you going to open a PR in mne-tools/mne-python?

ephathaway · 2020-11-09T19:46:35Z

@ephathaway Yeah I think that's pretty common for these kind of situations. Are you going to open a PR in mne-tools/mne-python?

It's been open for a while now. Gone through several rounds of review, but I think it's close. mne-tools#8354

jusjusjus · 2020-11-10T12:06:36Z

Looks good

Thanks. I still think we should merge this into mne-tools/mne-python master branch when it's approved and then pull those changes into our master.

Sure, I think we discussed in detail how we should have done it.

fork upstream/master -> (our) origin/master
develop in a branch origin/feature
PR origin/feature -> origin/master for internal review
rebase origin/master on top of upstream/master
PR origin/master -> upstream/master
rebase as needed until merged

It's at your discretion how to continue on the current path though. I'm happy to discuss options.

ephathaway · 2020-11-10T17:47:10Z

It's at your discretion how to continue on the current path though. I'm happy to discuss options.

Yeah, I think it makes sense to continue on the current path this time around since it's almost ready to merge. In my opinion, it's nice to submit the PR with origin/feature_branch -> upstream/master because then it's easy to push requested changes directly to origin/feature_branch instead of heaving to create a new branch every time we want to make changes if we were doing origin/master -> upstream/master.

Here we add a function `mne.io.read_evokeds_mff` to read averaged MFF files. This function works similarly to `mne.evoked.read_evokeds` by returning the averaged data and metadata from one or more specified `condition`(s). We make use of the mffpy[https://github.com/BEL-Public/mffpy] package for reading the signal data and some of the metadata in the MFF file.

Here we make some stylistic changes such as variable naming convention to make the code more readable and consistent with the MNE style. We implement two new private functions in `mne/io/egi/egimff.py` to replace some code that was redundant between class `RawMff` and `_read_evoked_mff`.

ephathaway requested review from ianbrown9475 and jusjusjus September 14, 2020 23:49

ianbrown9475 requested changes Sep 16, 2020

View reviewed changes

mne/io/egi/egimff.py Outdated Show resolved Hide resolved

mne/io/egi/egimff.py Outdated Show resolved Hide resolved

mne/io/egi/egimff.py Show resolved Hide resolved

mne/io/egi/egimff.py Outdated Show resolved Hide resolved

mne/io/egi/egimff.py Outdated Show resolved Hide resolved

ephathaway requested a review from ianbrown9475 September 18, 2020 00:06

jusjusjus requested changes Sep 21, 2020

View reviewed changes

jusjusjus approved these changes Sep 23, 2020

View reviewed changes

ianbrown9475 approved these changes Sep 23, 2020

View reviewed changes

jusjusjus approved these changes Oct 9, 2020

View reviewed changes

ianbrown9475 approved these changes Oct 9, 2020

View reviewed changes

ephathaway force-pushed the master branch from 43ca6ee to 6934034 Compare October 21, 2020 23:56

ephathaway force-pushed the read-mff-evoked branch 5 times, most recently from c60dad5 to 3ffb351 Compare October 28, 2020 19:27

ephathaway force-pushed the read-mff-evoked branch 3 times, most recently from 9bd1a44 to 981f199 Compare November 6, 2020 22:56

jusjusjus approved these changes Nov 9, 2020

View reviewed changes

ephathaway force-pushed the read-mff-evoked branch from 981f199 to 21cd4e9 Compare November 20, 2020 21:41

ephathaway and others added 26 commits November 23, 2020 12:00

STY: docstring and code style

831ef50

STY: new line at end of

77351b1

ENH: check if input file is averaged

0efb59f

ENH: Update mffpy to >=0.5.5

a115e23

ENH: import mffpy dynamically

6587209

ENH: Add version added string

e84f858

FIX: Fix evoked test to align mne policy

88257dc

FIX: organize testing data

35f9b22

ENH: apply reference info when reading MFF

d5bac43

DOC: update changelog

23caf0a

FIX: loose version for mffpy requirement

25cff7b

DOC: fix doc string style

af2f8f3

FIX: python ref for

2e5c31d

DOC: new contributor changelog entry

5bf9864

DOC: make parameter references render as code

685f2b4

Move test signals to mne-testing-data

5e9c207

ENH: check channel types and names

c46f121

FIX: require mffpy 0.5.5 for test

eff3858

FIX: pull in version bug fix in mffpy

0a9fa05

ENH: read nave from categories.xml

1b95cce

Improve test coverage

58290eb

DOC: fix version added

96b3aac

STY: use f-strings and replace asserts

6607349

ENH: test error messages for bad inputs

b71ab73

FIX: return list if condition is list

3487e6e

ephathaway force-pushed the read-mff-evoked branch from 21cd4e9 to 3487e6e Compare November 23, 2020 21:17

DOC: reference mffpy dependency in README

9b20666

ephathaway closed this Nov 24, 2020

ephathaway deleted the read-mff-evoked branch November 24, 2020 18:25

Read mff evoked #3

Read mff evoked #3

Uh oh!

Conversation

ephathaway commented Sep 14, 2020

Uh oh!

ianbrown9475 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ephathaway commented Sep 18, 2020

Uh oh!

jusjusjus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ephathaway commented Sep 22, 2020

Uh oh!

jusjusjus left a comment

Choose a reason for hiding this comment

Uh oh!

ianbrown9475 left a comment

Choose a reason for hiding this comment

Uh oh!

ephathaway commented Sep 24, 2020

Uh oh!

jusjusjus commented Sep 25, 2020

Uh oh!

jusjusjus left a comment

Choose a reason for hiding this comment

Uh oh!

ianbrown9475 left a comment

Choose a reason for hiding this comment

Uh oh!

jusjusjus left a comment

Choose a reason for hiding this comment

Uh oh!

ephathaway commented Nov 9, 2020

Uh oh!

ianbrown9475 commented Nov 9, 2020

Uh oh!

ephathaway commented Nov 9, 2020

Uh oh!

jusjusjus commented Nov 10, 2020

Uh oh!

ephathaway commented Nov 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants