Skip to content

Conversation

@massich
Copy link
Contributor

@massich massich commented Jan 23, 2019

split save and reload a FIF file fails due to Annotations

Fixes #5856

@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #5857 into master will decrease coverage by 1.77%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5857      +/-   ##
==========================================
- Coverage   89.23%   87.45%   -1.78%     
==========================================
  Files         417      417              
  Lines       75168    75176       +8     
  Branches    12376    12375       -1     
==========================================
- Hits        67075    65745    -1330     
- Misses       5215     6577    +1362     
+ Partials     2878     2854      -24

@larsoner
Copy link
Member

larsoner commented Feb 8, 2019

It would be great if this could also fix #5908

@larsoner larsoner added this to the 0.18 milestone Mar 27, 2019
@larsoner
Copy link
Member

@massich it would be good to release soon, this should go in if at all possible so this is a high priority!

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2019

This pull request introduces 1 alert when merging 5b35f1f into 017d156 - view on LGTM.com

new alerts:

  • 1 for Unused import

Comment posted by LGTM.com

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2019

This pull request introduces 2 alerts when merging e8609b8 into 14d0527 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'
  • 1 for Unused import

Comment posted by LGTM.com

@massich
Copy link
Contributor Author

massich commented May 15, 2019

The current status solves #5856 and does not break any other tests.

The following snipped fails in master and passes in the PR.

import os.path as op
import numpy as np
from numpy.testing import assert_array_equal, assert_allclose

from mne.datasets import testing
from mne.io import read_raw_fif
from mne.utils import _TempDir
from mne.annotations import Annotations

testing_path = testing.data_path(download=False)
data_dir = op.join(testing_path, 'MEG', 'sample')
fif_fname = op.join(data_dir, 'sample_audvis_trunc_raw.fif')
tempdir = _TempDir()

raw_1 = read_raw_fif(fif_fname, preload=True)
annot = Annotations(np.arange(20), np.ones((20,)), 'test')
raw_1.set_annotations(annot)
split_fname = op.join(tempdir, 'split_raw.fif')
raw_1.save(split_fname, buffer_size_sec=1.0, split_size='10MB')
raw_2 = read_raw_fif(split_fname)
assert_allclose(raw_2.buffer_size_sec, 1., atol=1e-2)  # samp rate
assert_allclose(raw_1.annotations.onset, raw_2.annotations.onset)
assert_allclose(raw_1.annotations.duration, raw_2.annotations.duration,
                rtol=0.001 / raw_2.info['sfreq'])
assert_array_equal(raw_1.annotations.description,
                    raw_2.annotations.description)

@massich massich changed the title [wip] FIX: save-load split FIF round trip [MRG] FIX: save-load split FIF round trip May 15, 2019
@larsoner
Copy link
Member

Probably worth an entry in whats_new.rst BUG section, otherwise +1 for merge, thanks @massich

@larsoner
Copy link
Member

The following snipped fails in master and passes in the PR.

This is effectively captured by one of the modifications to the test files now, right? If not, please do add it because it's an important non-regression test.

@massich
Copy link
Contributor Author

massich commented May 15, 2019

@larsoner yes it is. Actually, the snippet is just a snipped from the modified test.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

just add entry in what's new bug fixes.

I like fixes which are red lines :)

@larsoner larsoner merged commit 1567fef into mne-tools:master May 15, 2019
@larsoner
Copy link
Member

Thanks @massich !

@massich massich deleted the split_fif branch May 18, 2019 22:03
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.

save-load FIF round trip fails due to Annotations

3 participants