Skip to content

Conversation

@thht
Copy link
Contributor

@thht thht commented Feb 10, 2020

Reference issue

Fixes #7293

What does this implement/fix?

This PR fixes two bugs found when investigating #7293:

  1. The error message returned when the data structure is a cell array was not intuitive.
  2. When a channel was not present in the info object but in the loaded evoked data, the function would crash.

@thht thht changed the title [WIP] Fix handling of cell arrays and loading of evoked mat files when channels are not in info [MRG] Fix handling of cell arrays and loading of evoked mat files when channels are not in info Feb 10, 2020
@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #7304 into master will decrease coverage by 0.21%.
The diff coverage is 92.1%.

@@            Coverage Diff             @@
##           master    #7304      +/-   ##
==========================================
- Coverage   89.89%   89.68%   -0.22%     
==========================================
  Files         450      450              
  Lines       81284    81286       +2     
  Branches    12920    12923       +3     
==========================================
- Hits        73070    72900     -170     
- Misses       5391     5526     +135     
- Partials     2823     2860      +37

@thht
Copy link
Contributor Author

thht commented Feb 14, 2020

the azure pipelines fail with some access violation. no idea whats wrong there...

@thht
Copy link
Contributor Author

thht commented Feb 14, 2020

@agramfort should we rerun the tests?

@larsoner
Copy link
Member

Restarted but if it fails again you can ignore scalp_coupling failures if they are the cause, these will be fixed by #7319

@thht
Copy link
Contributor Author

thht commented Feb 14, 2020

ok, codecov complaining seems to come from the fact that pandas seems to be always available. is this correct?

@larsoner
Copy link
Member

It's not available on all builds, there is a minimal one. But that build also does not have h5py and a lot of other things so it might not hit all branches in your code

@larsoner
Copy link
Member

(this is fairly common though and not worth fighting here to deal with, we can live with it)

@thht
Copy link
Contributor Author

thht commented Feb 14, 2020

ok. so, from my side this is fine now. could you guys take a look and see if there is something i missed or still need to do?

@testing.requires_testing_data
# Reading the sample CNT data results in a RuntimeWarning because it cannot
# parse the measurement date. We need to ignore that warning.
@pytest.mark.filterwarnings('ignore::RuntimeWarning')
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a suitable regex in the :: so that it's more targeted? That way other warnings that we should pay attention to won't creep in easily.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Other than a minor ignores gripe, LGTM

@thht
Copy link
Contributor Author

thht commented Feb 17, 2020

ok, i added the chnages proposed by @larsoner and already added to one test by @agramfort to the other tests needing it.

CI failures seem to not come from my code....

@larsoner larsoner merged commit 478d38e into mne-tools:master Feb 17, 2020
@larsoner
Copy link
Member

Thanks @thht !

@thht thht deleted the fix_7293 branch February 17, 2020 16:01
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
…n channels are not in info (mne-tools#7304)

* added test for cell arrays

* added validation of fieldtrip structure

* make pydocstyle happy

* added test for bug when loading evoked mat files with missing channels

* fixed test

* also prune avg field

* pydocstring happy

* updated testing data version and hash

* fixed extra space

* updated whatsnew

* fix typo in whatsnew

* address comment from @larsoner

* use more precise warning filter in all tests

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
…n channels are not in info (mne-tools#7304)

* added test for cell arrays

* added validation of fieldtrip structure

* make pydocstyle happy

* added test for bug when loading evoked mat files with missing channels

* fixed test

* also prune avg field

* pydocstring happy

* updated testing data version and hash

* fixed extra space

* updated whatsnew

* fix typo in whatsnew

* address comment from @larsoner

* use more precise warning filter in all tests

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
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.

Error using mne.read_evoked_fieldtrip

3 participants