[MRG] Fixing quickstart doc bug, write_raw_bids doc bug, and coordsystem.json overwriting error #675

adam2392 · 2021-01-06T22:26:04Z

PR Description

Closes: #663
Closes: #662
Closes: #656

Currently to my knowledge, MEG data coordsystem.json file is only supported as one per session. There might be say multiple coordsystem.json files, but there is no right way to read those in currently. This addresses the main doc errors and FileExistsError.

Merge checklist

Maintainer, please confirm the following before merging:

codecov-io · 2021-01-06T22:33:18Z

Codecov Report

Merging #675 (2d934f6) into master (00e6c88) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
+ Coverage   93.69%   93.71%   +0.02%     
==========================================
  Files          22       22              
  Lines        2697     2707      +10     
==========================================
+ Hits         2527     2537      +10     
  Misses        170      170

Impacted Files	Coverage Δ
mne_bids/write.py	`96.99% <ø> (ø)`
mne_bids/dig.py	`91.19% <100.00%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00e6c88...a5f6897. Read the comment docs.

mne_bids/write.py

mne_bids/dig.py

adam2392 · 2021-01-07T19:02:13Z

CI is failing due to pulling data again.

Checked locally and things work at least for me.

agramfort · 2021-01-07T20:21:43Z

mne_bids/tests/test_write.py

+        ('CTF', 'testdata_ctf.ds', _read_raw_ctf, 'meg', ''),
+        ('MEG', 'sample/sample_audvis_trunc_raw.fif', _read_raw_fif, 'meg', ''),  # noqa
+    ]
+)


@adam2392 I don't have time to review in details but please monitor test time when you add tests. Add such big parametrize has a cost in CI time and needs to be fully justified. It's a general statement. I'd to avoid having mne-bids having a test time lasting tens of minutes. I find it already quite long actually

I ran these locally and it's actually quite fast because all these files are very very small. The chunk using write_raw_bids as a result is very fast.

The tests that generally seem to be taking long periods of time are actually the ones where we test for split of fif files correctly (need a humongous chunk to split it up > 2GB?), and the ones where we test the inspector. This is due to qualitative observation though, not an explicit profiling.

I can take these out, but I added these cases for the sake of covering a multiple coordsystems per datatype. Do we want to limit the parametrizations?

Actually realized I could do profiling with a one liner cuz I have worked with pytest-profiler before:

Profiling (from /Users/adam2392/Documents/mne-bids/prof/combined.prof): Thu Jan 7 16:19:56 2021 /Users/adam2392/Documents/mne-bids/prof/combined.prof 100182265 function calls (96919567 primitive calls) in 218.726 seconds Ordered by: cumulative time List reduced from 6828 to 20 due to restriction <20> ncalls tottime percall cumtime percall filename:lineno(function) 179 0.002 0.000 218.719 1.222 runner.py:106(pytest_runtest_protocol) 179 0.002 0.000 218.650 1.222 runner.py:114(runtestprotocol) 537 0.005 0.000 218.647 0.407 runner.py:212(call_and_report) 3506/1979 0.018 0.000 218.642 0.110 hooks.py:272(__call__) 3506/1979 0.003 0.000 218.627 0.110 manager.py:90(_hookexec) 3506/1979 0.008 0.000 218.625 0.110 manager.py:84(<lambda>) 3506/1979 0.046 0.000 218.619 0.110 callers.py:157(_multicall) 537 0.003 0.000 216.582 0.403 runner.py:240(call_runtest_hook) 537 0.006 0.000 216.573 0.403 runner.py:298(from_call) 537 0.002 0.000 216.564 0.403 runner.py:255(<lambda>) 179 0.001 0.000 215.183 1.202 runner.py:153(pytest_runtest_call) 179 0.001 0.000 215.179 1.202 python.py:1639(runtest) 179 0.272 0.002 215.169 1.202 python.py:176(pytest_pyfunc_call) 56 0.001 0.000 81.284 1.451 conftest.py:29(_validate) 56 0.007 0.000 81.283 1.451 <decorator-gen-2>:1(run_subprocess) 56 0.073 0.001 81.274 1.451 misc.py:90(run_subprocess) 7964 0.142 0.000 80.728 0.010 queue.py:153(get) 7265 0.110 0.000 80.532 0.011 threading.py:270(wait) 33516 80.368 0.002 80.368 0.002 {method 'acquire' of '_thread.lock' objects} 203 0.053 0.000 42.583 0.210 write.py:904(write_raw_bids) ======================================================================== slowest 20 durations ========================================================================= 35.23s call mne_bids/tests/test_read.py::test_write_read_fif_split_file 19.52s call mne_bids/tests/test_write.py::test_write_anat 8.63s call mne_bids/tests/test_write.py::test_fif 6.81s call mne_bids/tests/test_inspect.py::test_inspect_annotations 6.54s call mne_bids/tests/test_write.py::test_eegieeg[Persyst-sub-pt1_ses-02_task-monitor_acq-ecog_run-01_clip2.lay-fn] 6.37s call mne_bids/tests/test_inspect.py::test_inspect_set_and_unset_bads 6.27s call mne_bids/tests/test_write.py::test_eegieeg[NihonKohden-MB0400FU.EEG-fn] 6.23s call mne_bids/tests/test_inspect.py::test_inspect_annotations_remove_all 6.15s call mne_bids/tests/test_write.py::test_eegieeg[EDF-test_reduced.edf-fn] 5.64s call mne_bids/tests/test_write.py::test_set 5.56s call mne_bids/tests/test_inspect.py::test_inspect_bads_and_annotations 3.99s call mne_bids/tests/test_write.py::test_ctf 3.87s call mne_bids/tests/test_update.py::test_update_sidecar_jsons 3.84s call mne_bids/tests/test_inspect.py::test_inspect_single_file[True] 3.29s call mne_bids/tests/test_inspect.py::test_inspect_single_file[False] 3.28s call mne_bids/tests/test_write.py::test_mark_bad_channels[MEG 0112-Really bad!-True-True-existing_ch_names7-existing_descriptions7-None-False] 3.19s call mne_bids/tests/test_write.py::test_mark_bad_channels[ch_names0-None-False-False-existing_ch_names0-existing_descriptions0-None-False] 3.19s call mne_bids/tests/test_write.py::test_write_meg_calibration 3.18s call mne_bids/tests/test_write.py::test_mark_bad_channels[ch_names8-descriptions8-False-False-existing_ch_names8-existing_descriptions8-None-False] 3.17s call mne_bids/tests/test_write.py::test_mark_bad_channels[ch_names6-descriptions6-False-False-existing_ch_names6-existing_descriptions6-meg-False]

If anything, I think the test_eegieeg I parametrized could be widdled down (if possible). Otw, these other tests are at least for now the longest 20.

mne_bids/dig.py

doc/whats_new.rst

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

mne_bids/tests/test_write.py

mne_bids/dig.py

adam2392 · 2021-01-09T00:28:13Z

This was brought up by me awhile back and I suppose at this point it depends what "version" of BIDs we're goig with:

https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/03-electroencephalography.html#electrodes-description-_electrodestsv

Ref issue: bids-standard/bids-specification#649

@sappelhoff thoughts?

sappelhoff · 2021-01-09T08:47:57Z

I would adopt bids latest version for this case. Because we are releasing soon.

adam2392 · 2021-01-09T16:40:17Z

Sweet! Then things are okay on my end. Idk why but one of the CI always fails fetching data :(

mne_bids/dig.py

hoechenberger · 2021-01-13T18:51:09Z

Thanks @adam2392!

Fixing doc bugs.

40bca42

adam2392 mentioned this pull request Jan 6, 2021

Documentation bug in quickstart guide #663

Closed

hoechenberger requested changes Jan 7, 2021

View reviewed changes

mne_bids/write.py Outdated Show resolved Hide resolved

Adding tests.

2765c1b

hoechenberger reviewed Jan 7, 2021

View reviewed changes

mne_bids/dig.py Outdated Show resolved Hide resolved

This was referenced Jan 7, 2021

?Bug? Setting landmark coordinates in coord_frame 'mri' for ecog/seeg channels results in Captrak coordframe #676

Open

iEEGCoordinateSystemDescription is not being written correctly #677

Closed

adam2392 added 2 commits January 7, 2021 13:34

Fixing coverage.

444e048

Adding whatsnew.

2a8ba6c

agramfort reviewed Jan 7, 2021

View reviewed changes

hoechenberger requested changes Jan 7, 2021

View reviewed changes

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

mne_bids/dig.py Outdated Show resolved Hide resolved

doc/whats_new.rst Outdated Show resolved Hide resolved

adam2392 and others added 5 commits January 7, 2021 16:00

Update doc/whats_new.rst

ba40dfd

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne_bids/dig.py

30cd8f1

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne_bids/dig.py

3da2805

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne_bids/dig.py

8402893

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Fixing runtimeerror string.

eb575b4

hoechenberger reviewed Jan 7, 2021

View reviewed changes

mne_bids/tests/test_write.py Outdated Show resolved Hide resolved

adam2392 mentioned this pull request Jan 7, 2021

[maint] Possible refactor/organization of unit tests for readability and decreased time (if possible) #678

Open

adam2392 added 4 commits January 7, 2021 18:26

Add test to eeg/ieeg as well.

c887113

Fixing unit test.

27ede5b

Fixing whatsnew.

795a44b

Fixing whatsnew.

ebf8567

hoechenberger reviewed Jan 8, 2021

View reviewed changes

mne_bids/dig.py Show resolved Hide resolved

adam2392 added 2 commits January 8, 2021 09:49

Fixing oneliner.

9249da0

Fixing unit tests.

2d934f6

hoechenberger requested changes Jan 8, 2021

View reviewed changes

mne_bids/dig.py Show resolved Hide resolved

agramfort approved these changes Jan 10, 2021

View reviewed changes

adam2392 requested a review from hoechenberger January 13, 2021 17:03

hoechenberger reviewed Jan 13, 2021

View reviewed changes

mne_bids/dig.py Outdated Show resolved Hide resolved

hoechenberger approved these changes Jan 13, 2021

View reviewed changes

Update mne_bids/dig.py

a5f6897

hoechenberger merged commit 9b82c86 into mne-tools:master Jan 13, 2021

adam2392 deleted the doc branch January 13, 2021 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Fixing quickstart doc bug, write_raw_bids doc bug, and coordsystem.json overwriting error #675

[MRG] Fixing quickstart doc bug, write_raw_bids doc bug, and coordsystem.json overwriting error #675

adam2392 commented Jan 6, 2021

codecov-io commented Jan 6, 2021 •

edited

Loading

adam2392 commented Jan 7, 2021

agramfort Jan 7, 2021

adam2392 Jan 7, 2021

adam2392 Jan 7, 2021

adam2392 Jan 7, 2021 •

edited

Loading

adam2392 commented Jan 9, 2021

sappelhoff commented Jan 9, 2021

adam2392 commented Jan 9, 2021

hoechenberger commented Jan 13, 2021

[MRG] Fixing quickstart doc bug, write_raw_bids doc bug, and coordsystem.json overwriting error #675

[MRG] Fixing quickstart doc bug, write_raw_bids doc bug, and coordsystem.json overwriting error #675

Conversation

adam2392 commented Jan 6, 2021

PR Description

Merge checklist

codecov-io commented Jan 6, 2021 • edited Loading

Codecov Report

adam2392 commented Jan 7, 2021

agramfort Jan 7, 2021

Choose a reason for hiding this comment

adam2392 Jan 7, 2021

Choose a reason for hiding this comment

adam2392 Jan 7, 2021

Choose a reason for hiding this comment

adam2392 Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

adam2392 commented Jan 9, 2021

sappelhoff commented Jan 9, 2021

adam2392 commented Jan 9, 2021

hoechenberger commented Jan 13, 2021

codecov-io commented Jan 6, 2021 •

edited

Loading

adam2392 Jan 7, 2021 •

edited

Loading