MRG, ENH: Use datetime objects for meas_date #7124

larsoner · 2019-12-05T14:39:41Z

~~Closes #7116.~~

~~First step, replicate the failure (hopefully). It passes locally on Linux but hopefully Azure shows the issue.~~

Use datetime | None objects for info['meas_date'].
Use datetime | None for raw.annotations.orig_time
Add code for set_meas_date that takes care of annotations and such. The anonymization code can use this / be refactored to use it. It only takes datetime | None as an argument.
~~Extend set_meas_date and/or anonymize functions to kill some of these other dates (such as meas_id)~~
Fix bug where anonymizing would shift annotations by raw._first_time
Fix problems in Windows (fromtimestamp)

larsoner · 2019-12-05T15:07:58Z

This is on its way to passing because the bug was already fixed in #7036. I'll re-title and eventually make this into a PR to use datetime objects with meas_date (hopefully).

codecov · 2019-12-05T22:11:14Z

Codecov Report

Merging #7124 into master will increase coverage by 0.01%.
The diff coverage is 91.45%.

@@            Coverage Diff             @@
##           master    #7124      +/-   ##
==========================================
+ Coverage    89.7%   89.71%   +0.01%     
==========================================
  Files         444      444              
  Lines       78997    79091      +94     
  Branches    12674    12689      +15     
==========================================
+ Hits        70863    70956      +93     
- Misses       5320     5322       +2     
+ Partials     2814     2813       -1

larsoner · 2019-12-06T17:31:49Z

FYI I found a bug in master. This code:

import numpy as np
import mne
from mne.annotations import _handle_meas_date

meas_date = (1, 2)
raw = mne.io.RawArray(10e-6 * np.sin(np.linspace(0, 2*np.pi, 10000))[None],
                      mne.create_info(1, 1000., 'eeg'), first_samp=2000)
try:  # PR
    raw.set_meas_date(meas_date)
except:  # master
    raw.info['meas_date'] = meas_date
    raw.annotations.orig_time = _handle_meas_date(meas_date)
raw.annotations.append(3 + raw._first_time, 1, '3-4')
raw.plot()
raw.anonymize()
raw.plot()

Does something bad to the annotation:

It's fixed in this PR.

And then I noticed that this same code does not produce an annotation in the same place when meas_date=None at the top because of how we treat annotations in that case (the raw._first_time had to be there if and only if meas_date was not None), which seems like a bug. Removing it makes tests and code more consistent, and much simpler.

larsoner · 2019-12-06T17:32:18Z

Tests pass locally, this is ready for review/merge from my end

larsoner · 2019-12-06T17:53:54Z

mne/io/tests/test_raw.py

    raw = _raw_annot(None, None)
    assert raw.annotations.orig_time is None
-    assert raw.annotations.onset[0] == 0.5
+    assert raw.annotations.onset[0] == 1.5


@agramfort this is the line that should match above

larsoner · 2019-12-06T17:55:44Z

mne/io/base.py

+                                     emit_warning=emit_warning)
+                new_annotations.onset -= (
+                    meas_date - new_annotations.orig_time).total_seconds()
+            new_annotations._orig_time = meas_date


Hopefully simpler

larsoner · 2019-12-06T17:56:00Z

mne/annotations.py

+    assert two is not None
+    shift = one_n_samples / sfreq  # to the right by the number of samples
+    shift += one_first_samp / sfreq  # to the right by the offset
+    shift -= two_first_samp / sfreq  # undo its offset


Hopefully simpler

mne/channels/channels.py

doc/changes/latest.inc

mne/io/brainvision/brainvision.py

agramfort · 2019-12-08T12:23:07Z

mne/io/brainvision/tests/test_brainvision.py

    """Test load brainvision annotations and parse them to events."""
    sfreq = 1000.0
-    expected_orig_time = 1384359243.794231
+    expected_orig_time = _stamp_to_dt((1384359243, 794232))


why do you have all these changes by 1uS ?

I think in principle it should be more accurate now, as it uses round rather than int cast. These values I'm assuming were taken from what was produced by the old code, not necessarily what was most correct.

mne/io/ctf/tests/test_ctf.py

agramfort · 2019-12-08T12:24:24Z

mne/io/ctf/tests/test_ctf.py

    events, _ = events_from_annotations(raw)
    latencies = np.sort(events[:, 0])
-    assert_array_equal(latencies, EXPECTED_LATENCIES)
+    assert_allclose(latencies, EXPECTED_LATENCIES, atol=1e-6)


Same as above I think, we were operating at the precision limit before. Rather than change all values in EXPECTED_LATENCIES I just made it tolerant of a 1 μs error.

mne/io/tests/test_meas_info.py

agramfort · 2019-12-08T12:34:23Z

mne/tests/test_annotations.py

-    assert_allclose(raw.annotations.duration, duration)
-    assert_allclose(raw_concat.annotations.duration, duration)
+    assert_allclose(raw.annotations.duration, duration, atol=1e-6)
+    assert_allclose(raw_concat.annotations.duration, duration, atol=1e-6)


is this necessary? do we have a loss of precision with this change?

This one I can push a little fix for

agramfort

thx @larsoner

feel free to merge when CIs come back green

* TST: Add test for another meas_date * ENH: Use datetime instead of float * FIX: Fwd * FIX: Simplifications * ENH: Add set_meas_date * FIX: Unify treatment of first_samp * FIX: Ref * FIX: Ref * FIX: Closer * FIX: Replicate and fix * Update test_annotations.py * FIX: Tweak duration

TST: Add test for another meas_date [ci skip]

c1f3ee5

larsoner changed the title ~~WIP, FIX: Fix meas_date info repr~~ WIP: Use datetime objects for meas_date Dec 5, 2019

ENH: Use datetime instead of float

ac7b231

larsoner added 4 commits December 6, 2019 09:30

FIX: Fwd

2de4355

FIX: Simplifications

fdf8e6f

ENH: Add set_meas_date

b305ca5

FIX: Unify treatment of first_samp

4606f64

larsoner changed the title ~~WIP: Use datetime objects for meas_date~~ ENH, FIX: Use datetime objects for meas_date Dec 6, 2019

larsoner changed the title ~~ENH, FIX: Use datetime objects for meas_date~~ MRG, ENH: Use datetime objects for meas_date Dec 6, 2019

larsoner commented Dec 6, 2019

View reviewed changes

larsoner added 2 commits December 6, 2019 12:58

FIX: Ref

6a45231

FIX: Ref

8f4a2e8

jasmainak reviewed Dec 6, 2019

View reviewed changes

mne/channels/channels.py Outdated Show resolved Hide resolved

larsoner added 3 commits December 6, 2019 15:40

FIX: Closer

9da4ae7

FIX: Replicate and fix

6b8b0bd

Update test_annotations.py

9347746

agramfort reviewed Dec 8, 2019

View reviewed changes

FIX: Tweak duration

de3446a

agramfort approved these changes Dec 8, 2019

View reviewed changes

larsoner merged commit b3f22f9 into mne-tools:master Dec 8, 2019

larsoner deleted the meas_date branch December 8, 2019 19:33

agramfort mentioned this pull request Dec 8, 2019

support new meas_date of type datetime in mne master mne-tools/mne-bids#326

Merged

SveaMeyer13 mentioned this pull request Apr 8, 2020

MRG, FIX: Datetime call in gdf 2.x age calculation #7581

Merged

Uh oh!

MRG, ENH: Use datetime objects for meas_date #7124

MRG, ENH: Use datetime objects for meas_date #7124

Uh oh!

Conversation

larsoner commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Dec 5, 2019

Uh oh!

codecov bot commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

larsoner commented Dec 6, 2019

Uh oh!

larsoner commented Dec 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

larsoner commented Dec 5, 2019 •

edited

Loading

codecov bot commented Dec 5, 2019 •

edited

Loading