Skip to content

Conversation

@massich
Copy link
Contributor

@massich massich commented Nov 5, 2018

Fixes #5670

Things to do:

  • migrate the calls of:
    • brainstorm
    • brainvision
    • EDF
    • fif
    • eeglab
  • Depreacate read_annotations_eeglab since it was in 0.16
    rst - Add function :func:`mne.io.read_annotations_eeglab` to allow loading annotations from EEGLAB files, by `Alex Gramfort`_`

with ff as fid:
annotations = _read_annotations(fid, tree)
# XXX: I've issues with circular imports
from mne.io.brainvision.brainvision import _read_annotations_brainvision_caller
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the _caller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. What I think it should be done is unify them all.
I would propose to remove all the Annotation construction of the readers and delegate it here to mne.annotations.read_annotations. In this manner, all readers should have a function _read_annotations_XX (or parse or whatever) which does know about the file format and returns valid onsets, duration, description and orig_time. And remove the import Annotations.

However, this would be easier when read_annotations_eeglab is no longer there, and the synthesis of the stim channel is removed from the edf reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but meanwhile, I'll just remove the callers.

@agramfort
Copy link
Member

agramfort commented Nov 5, 2018 via email

@codecov
Copy link

codecov bot commented Nov 5, 2018

Codecov Report

Merging #5695 into master will decrease coverage by <.01%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##           master    #5695      +/-   ##
==========================================
- Coverage   88.51%   88.51%   -0.01%     
==========================================
  Files         369      369              
  Lines       69136    69148      +12     
  Branches    11645    11651       +6     
==========================================
+ Hits        61193    61203      +10     
- Misses       5089     5090       +1     
- Partials     2854     2855       +1

@massich massich changed the title [RFC] Unify read annotations [WIP] Unify read annotations Nov 5, 2018
@massich massich changed the title [WIP] Unify read annotations [RFC] Unify read annotations Nov 5, 2018
@massich
Copy link
Contributor Author

massich commented Nov 5, 2018

If you guys think that:

  • the RFC, are not important
  • CIs are green
  • Two people +1

then feel free to merge.

@larsoner
Copy link
Member

larsoner commented Nov 5, 2018

CIs are not happy, let me know when they are and I can take a look

@larsoner larsoner added this to the 0.17 milestone Nov 5, 2018
@larsoner
Copy link
Member

larsoner commented Nov 5, 2018

Marking this for 0.17

@massich massich changed the title [RFC] Unify read annotations [MRG] Unify read annotations Nov 6, 2018
@larsoner
Copy link
Member

larsoner commented Nov 6, 2018

Is the TODO list at the top outdated, as there are things incomplete in the list

@massich massich changed the title [MRG] Unify read annotations [MRG+1] Unify read annotations Nov 6, 2018
@massich massich merged commit ae38e1b into mne-tools:master Nov 6, 2018
@massich massich deleted the fix_annotations branch November 9, 2018 08:43
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.

4 participants