MRG: Add `units` parameter to get_data for Raw by HanBnrd · Pull Request #9136 · mne-tools/mne-python

HanBnrd · 2021-03-17T18:27:56Z

Reference issue

Partially addresses #8473:

Add units to Raw.get_data
Add units to Epochs.get_data

What does this implement/fix?

Adds a units parameter to the get_data() function of base.py.

With units being either None, str or dict.

Additional information

Continuation of #8476.

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

HanBnrd · 2021-03-18T17:06:55Z

Hi @sappelhoff @larsoner,

I ended up with an implementation in base.py, does this look good to you?
Should I copy paste the unit conversion bit to epochs.py?
Also I was wondering where I could write the tests for this? I didn't find any test_get_data().

larsoner · 2021-03-18T19:23:27Z

I would do it in a new mne/io/tests/test_raw.py:test_get_data_units function.

mne/io/tests/test_raw.py

sappelhoff

some minor comments, but provided that the CIs pass, I am +1 for merging. Thanks a lot @HanBnrd

mne/io/base.py

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

hoechenberger

Few nitpicks, otherwise LGTM

mne/io/base.py

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

mne/io/base.py

mne/io/tests/test_raw.py

agramfort · 2021-03-28T16:52:59Z

no but it should be tested as it's advertised as supported :)

…

hoechenberger · 2021-03-28T16:55:17Z

no but it should be tested as it's advertised as supported :)

Heh. Neat :) 😁

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

agramfort

+1 to MRG if CIs are green

larsoner · 2021-03-29T18:18:29Z

Does it make sense to refactor mne/tests/test_defaults.py:test_si_units to make use of the new conversion code? I suspect there are some redundancies now

larsoner · 2021-03-29T18:18:50Z

(also our CIs are failing on all PRs due to a matplotlib issue so feel free to ignore the raw plotting errors)

larsoner

LGTM +1 for merge assuming CIs are happy (aside from matplotlib failures)!

HanBnrd · 2021-03-29T19:34:39Z

Does it make sense to refactor mne/tests/test_defaults.py:test_si_units to make use of the new conversion code? I suspect there are some redundancies now

@larsoner, I deleted a good chuck of it as most of the tests were already done in _get_scaling

HanBnrd · 2021-03-29T19:36:05Z

LGTM +1 for merge assuming CIs are happy (aside from matplotlib failures)!

Should I merge main again once the CI is fixed?

larsoner · 2021-03-29T19:41:54Z

In theory you shouldn't need to, if we just restart the CIs at that point they should get the fix because they build a merged-with-main version of the PR. But you could also merge with main in your branch and it would work without needing to manually restart

hoechenberger · 2021-03-30T18:38:21Z

Thank you, @HanBnrd!!

sappelhoff and others added 6 commits March 15, 2021 19:12

make docstr proposal

b937739

Update mne/io/base.py [skip ci]

21e7e5c

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Draft argument for units in get_data

6e6d610

Merge branch 'main' into get-data-units-code-sprint

84566d3

Handle fractions and powers with str

b4e8ba8

Fix error message

1fe47f5

sappelhoff mentioned this pull request Mar 18, 2021

Add units param to get_data methods in Raw and Epochs #8476

Closed

sappelhoff added ENH MEDIUM labels Mar 18, 2021

sappelhoff assigned HanBnrd Mar 18, 2021

HanBnrd added 2 commits March 18, 2021 15:49

Add all checks for str argument

8e45051

Make it work with a dict

cc43d8a

Fix bugs

7c7608b

HanBnrd marked this pull request as ready for review March 18, 2021 17:52

HanBnrd added 5 commits March 19, 2021 06:48

Handle str ch picks and deci prefix

92c7389

Draft tests

d575b2d

Add tests

b1c5815

Add tests for shape

cb56d85

Merge branch 'main' into get-data-units-code-sprint

b5cb086

HanBnrd changed the title ~~WIP: Add units parameter to get_data~~ Add units parameter to get_data Mar 19, 2021

HanBnrd added 7 commits March 19, 2021 20:08

Update note about csd

94383f8

Fix flake8

6ef08f4

Fix doc

3c2a734

Merge branch 'main' into get-data-units-code-sprint

712c0da

Describe changes

d2f5ad0

Fix array shape

1e5d8d2

Merge branch 'main' into get-data-units-code-sprint

f241fef

HanBnrd changed the title ~~Add units parameter to get_data~~ Add units parameter to get_data for Raw Mar 20, 2021

HanBnrd commented Mar 27, 2021

View reviewed changes

mne/io/tests/test_raw.py Show resolved Hide resolved

HanBnrd requested a review from agramfort March 27, 2021 21:20

sappelhoff approved these changes Mar 27, 2021

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

mne/io/base.py Outdated Show resolved Hide resolved

mne/io/base.py Outdated Show resolved Hide resolved

Apply suggestions for base.py docstring

7f966cb

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

hoechenberger reviewed Mar 27, 2021

View reviewed changes

HanBnrd and others added 2 commits March 28, 2021 11:06

Apply suggestions from code review

ba9ee7b

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

Fix docstring

217dea6

HanBnrd commented Mar 28, 2021

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

agramfort reviewed Mar 28, 2021

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

mne/io/base.py Outdated Show resolved Hide resolved

mne/io/base.py Outdated Show resolved Hide resolved

mne/io/base.py Outdated Show resolved Hide resolved

mne/io/tests/test_raw.py Show resolved Hide resolved

HanBnrd and others added 3 commits March 28, 2021 19:16

Implement suggestions from code review

72b1419

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

Merge branch 'main' into get-data-units-code-sprint

b3968a5

Fix SI units doc

9e5ea86

agramfort approved these changes Mar 28, 2021

View reviewed changes

Merge branch 'main' into get-data-units-code-sprint

952d50c

Refactor test_si_units and fix edge cases

8867c35

HanBnrd requested a review from larsoner March 29, 2021 19:31

larsoner approved these changes Mar 29, 2021

View reviewed changes

Merge branch 'main' into get-data-units-code-sprint

4656f57

hoechenberger merged commit 98fb5c2 into mne-tools:main Mar 30, 2021

HanBnrd deleted the get-data-units-code-sprint branch March 30, 2021 19:22

sappelhoff mentioned this pull request Jul 12, 2021

[MRG] Add units parameter to get_data for Epochs #9553

Merged

6 tasks

drammock mentioned this pull request Sep 29, 2021

MRG, BUG: Fix bug with dipole raw data #9771

Merged

Uh oh!

Conversation

HanBnrd commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference issue

What does this implement/fix?

Additional information

Uh oh!

HanBnrd commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Mar 18, 2021

Uh oh!

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hoechenberger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agramfort commented Mar 28, 2021 via email

Uh oh!

hoechenberger commented Mar 28, 2021

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Mar 29, 2021

Uh oh!

larsoner commented Mar 29, 2021

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

HanBnrd commented Mar 29, 2021

Uh oh!

HanBnrd commented Mar 29, 2021

Uh oh!

larsoner commented Mar 29, 2021

Uh oh!

hoechenberger commented Mar 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HanBnrd commented Mar 17, 2021 •

edited

Loading

HanBnrd commented Mar 18, 2021 •

edited

Loading