Skip to content

Conversation

@massich
Copy link
Contributor

@massich massich commented Oct 15, 2018

From the whish list in #5201 it mainly addresses point 4.

@massich massich changed the title Add tutorial [WIP] Add events/annotations tutorial Oct 15, 2018
@massich massich force-pushed the annotation_tutorial branch from b2dc85f to 180620d Compare October 17, 2018 09:38
@codecov
Copy link

codecov bot commented Oct 17, 2018

Codecov Report

Merging #5608 into master will increase coverage by 0.01%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #5608      +/-   ##
==========================================
+ Coverage   88.55%   88.57%   +0.01%     
==========================================
  Files         369      369              
  Lines       69182    69194      +12     
  Branches    11653    11654       +1     
==========================================
+ Hits        61266    61286      +20     
+ Misses       5060     5056       -4     
+ Partials     2856     2852       -4

@larsoner
Copy link
Member

larsoner commented Nov 5, 2018

@massich do we need this for 0.17? If so it's likely to be one of the last blockers for release

@larsoner larsoner added this to the 0.17 milestone Nov 5, 2018
@massich
Copy link
Contributor Author

massich commented Nov 6, 2018

yes, we need this one. And goes after #5695

It is nice to have. you moved #5201 to 0.18 already. These 2 go together.

@massich massich modified the milestones: 0.17, 0.18 Nov 6, 2018
@massich massich force-pushed the annotation_tutorial branch from 48a7308 to 80c6966 Compare November 9, 2018 13:02
@massich
Copy link
Contributor Author

massich commented Nov 9, 2018

From the whish list in #5201 it mainly addresses point 4 which is the urgent part to get in 0.17. (So I'll edit the PR description)

@agramfort
Copy link
Member

@massich massich changed the title [WIP] Add events/annotations tutorial [MRG] Add events/annotations tutorial Nov 9, 2018
@massich
Copy link
Contributor Author

massich commented Nov 9, 2018

There's still one broken link.

@massich
Copy link
Contributor Author

massich commented Nov 9, 2018

I don't understand why this link does not work and it does not appear in the build as broken

ref:`sphx_glr_auto_tutorials_plot_artifacts_correction_rejection.py`

@massich
Copy link
Contributor Author

massich commented Nov 9, 2018

its the same as this is linking to. But here renders properly. I don't get it.

@@ -0,0 +1,134 @@
"""
.. _tut_events_and_annotation_objects:
Copy link
Member

Choose a reason for hiding this comment

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

remove this label, we do not need it (it's in other examples for historical reasons and those should be removed, too)

we can use the auto-generated sphx_glr_... labels instead

Events and annotations are quite similar. This tutorial highlights their
differences and similitudes and tries to shade some light to which one is
preferred to use in different situations when using MNE.
Here follows both terms definition from the glossary.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add a RST link to the glossary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes but I did not find how. I don't think is referenceable yet.

events
Events correspond to specific time points in raw data; e.g., triggers,
experimental condition events, etc. MNE represents events with integers
that are stored in numpy arrays of shape (n_events, 3). Such arrays are
Copy link
Member

Choose a reason for hiding this comment

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

I am always forget what the second column of the events array is. Maybe it's worth repeating here what the three columns represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this you need an example where the second column is more than array of 0s. And is not yet covered in this tutorial.

Events correspond to specific time points in raw data; e.g., triggers,
experimental condition events, etc. MNE represents events with integers
that are stored in numpy arrays of shape (n_events, 3). Such arrays are
classically obtained from a trigger channel, also referred to as stim
Copy link
Member

Choose a reason for hiding this comment

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

stim is really an abbreviation. Since it's a tutorial, I'd suggest expanding to the full name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only goal here was just to replicate whatever is in glossary. Once glossary is linkable I would remove it.

classically obtained from a trigger channel, also referred to as stim
channel.

annotations
Copy link
Member

@jasmainak jasmainak Nov 10, 2018

Choose a reason for hiding this comment

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

Link to documentation of annotations class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is linked everywhere, but here 'cos this is a citation of the glossary:
image

@massich massich force-pushed the annotation_tutorial branch from b18b962 to a9c0e66 Compare November 12, 2018 16:33
@massich
Copy link
Contributor Author

massich commented Nov 12, 2018

I think I've addressed everybody's concerns. What I did not manage is to link to the annotations attributes. (ie: :attr:orig_time <mne.Annotations.orig_time>) To do so, the Annotations parameters should be replicated under the title Attributres ('cos we need them as Parameters and Attributes) but even then the corssreferencing was not working for me. So I gave up. If someone is more skilled than me please push in this branch or make a PR in my fork.

@larsoner yes, they are inconsistent. I did not check if the inconsistency was released already. But I was certainly planning to correct it after the release due to #5699. And maybe find a better name for _handle_meas_date and expose it.

Please, give a read to the tutorial. Thx a lot.

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.

Otherwise LGTM


Both events and :class:`Annotations <mne.Annotations>` be seen as triplets
where the first element answers to **when** something happens and the last
element refers to **what** is it.
Copy link
Member

Choose a reason for hiding this comment

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

what it is

the description is an integer value.
In contrast, :class:`Annotations <mne.Annotations>` represents the
``onset`` in seconds (relative to the reference ``orig_time``),
and the ``description`` is an arbitrary string.
Copy link
Member

Choose a reason for hiding this comment

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

Remove leading space

# Create an annotation object with orig_time
orig_time = '2002-12-03 19:01:31.676071'
my_datetime = datetime.strptime(orig_time, '%Y-%m-%d %H:%M:%S.%f')
posix_timestamp = (my_datetime - datetime(1970, 1, 1)).total_seconds()
Copy link
Member

Choose a reason for hiding this comment

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

this is not used in the annotations object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I used only for printing. There's a subtiliy. the orig_time has to be .6760709 otherwise they wont match.

Copy link
Member

Choose a reason for hiding this comment

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

well okay but you don't use this orig_time either ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

print(posix_timestamp - annot_orig.orig_time)
1.1920928955078125e-07

I am using the same number. (except the rounding error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1038942091.676071 is the POSIX timestamp of 2002-12-03 19:01:31.676071

it only differs in the .6760709

Copy link
Member

@jasmainak jasmainak Nov 12, 2018

Choose a reason for hiding this comment

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

okay maybe then it's a matter of renaming your variables. Because I see this:

orig_time = '2002-12-03 19:01:31.676071'

and I'm confused why in the class instantiation you do: orig_time=1038942091.6760709.

also how much does the 1e-7 matter? couldn't we live with a slightly offset annotation with 1 sample?

Copy link
Member

Choose a reason for hiding this comment

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

@jasmainak I agree it's probably too much to ask to our user to understand this gymnastic of dates.
I am going to push now a way to pass directly '2002-12-03 19:01:31.676071' to the Annotations constructor.

Copy link
Member

Choose a reason for hiding this comment

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

actually I need #5699 first to do this... let me switch to #5699 then.


annot = mne.Annotations(onset=[10], duration=[0.5],
description=['foobar'],
orig_time=1038942091.6760709)
Copy link
Member

Choose a reason for hiding this comment

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

still to be fixed ...

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 don't see this timestamp as a problem. At least now it is stated that it has been 'chosen' and that is not completely arbitrary. But shows that orig_time is under the hood this kind of ugly floats.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry to insist here.

But if a user wanted to use the Annotations class, how do you propose that they would set orig_time? I don't think anyone could dream up this number. If the idea to expose this parameter is that allows people to adjust the orig_time if it's not aligned. You could perhaps make it annot.orig_time - 0.2 to shift orig time ...

Copy link
Member

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

Except those last two comments LGTM. Thanks @massich !

@massich
Copy link
Contributor Author

massich commented Nov 12, 2018

Onsets no longer match:

[42.95597088 44.95597088 51.95597088]
[42.955971 44.955971 51.955971]

We need to be able to parse orig_time until there's no difference in the delta time.
We should be able to do orig_time='2002-12-03 19:01:31.6760709' and it should take it. There should be a manner to tweak the strptime regex '%Y-%m-%d %H:%M:%S.%f' to take it. I did try when writting a9c0e66 I guess that I've to try harder. We'll find it.

@agramfort
Copy link
Member

agramfort commented Nov 12, 2018 via email

@larsoner
Copy link
Member

I pushed a commit that at least makes things pass on both systems. Had to use assert_allclose though. Maybe rounding is better

@larsoner
Copy link
Member

larsoner commented Nov 13, 2018

(I'm talking about pushing ee812c8, so over on #5699)

@larsoner
Copy link
Member

Pushed a better version that did not necessitate using assert_allclose (760640e)

@massich massich dismissed jasmainak’s stale review November 13, 2018 13:00

Done via ISO8601

@massich
Copy link
Contributor Author

massich commented Nov 13, 2018

It should be able to go in once green.

@larsoner
Copy link
Member

@jasmainak feel free to merge when you are happy

@jasmainak jasmainak merged commit df832f4 into mne-tools:master Nov 13, 2018
@jasmainak
Copy link
Member

Looks really nice. Thanks @massich and @agramfort for the teamwork !

@massich massich deleted the annotation_tutorial branch November 13, 2018 15:52
@agramfort
Copy link
Member

agramfort commented Nov 13, 2018 via email

annotated_blink_raw = raw.copy()
eog_events = mne.preprocessing.find_eog_events(raw)
n_blinks = len(eog_events)
# Center to cover the whole blink with full duration of 0.5s:
Copy link
Member

Choose a reason for hiding this comment

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

this may be a bit misleading - the events are centered and we are actually de-cenetering them by pushing them back in time to mark onset, not event center.

@agramfort
Copy link
Member

agramfort commented Nov 13, 2018 via email

@mmagnuski
Copy link
Member

@agramfort sure thing, I have some other small suggestions. :)

@agramfort
Copy link
Member

agramfort commented Nov 13, 2018 via email

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.

6 participants