SourceTFR by DiGyt · Pull Request #6543 · mne-tools/mne-python

DiGyt · 2019-07-09T18:16:51Z

This is a draft PR for a tentative Version of a SourceTFR class which is used to store time-frequency transformed Source Estimates. It does not yet cover some relevant functions (i.e. plotting or reading from disc).

This PR was mainly created, so @massich can review the code.

Added Basic SourceTFR structure and Text Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Updated SourceTFR to version used for reading morlet/multitapers Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

massich · 2019-07-12T16:09:33Z

mne/source_tfr.py

+#
+# Authors: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>
+# TODO: This file uses a lot of stuff from io/base.py and source_estimate.py
+# TODO: Maybe name these persons as authors too?


to me, the authors are the list of people that would know something about that file. It is the people I'm going to send them an email when things go south.

People who wrote the file or closely reviewed.

massich · 2019-07-12T16:10:50Z

mne/source_tfr.py

+        if not (isinstance(vertices, np.ndarray) or
+                isinstance(vertices, list)):
+            raise ValueError('Vertices must be a numpy array or a list of '
+                             'arrays')


we have functions for checking parameters.

massich · 2019-07-12T16:19:29Z

mne/source_tfr.py

+    """
+
+    @verbose
+    def __init__(self, data, vertices=None, tmin=None, tstep=None,


break the __init__ with functions. I know that some of our inits are huge, but they should be short easy to read in a glance.

def __init__(foo, bar, data, vertices, bla): _check_param(foo, ...) self._data = _prepare_data(data) self._vercies = _foo(vertices) ...

to me init should be just assigning stuff to the attributes. If _prepare_data has a signature like _prepare_data(data=data, n_col=vertices.shape[0], moon_color=foo) it tels me at a glance how data is related to the other things. Sure I would not know the details but I least I would know the possible relations. If I have to read the code and strip all the value errors out on my mind the task of reading the code is much harder.

keep in mind: code is written once but read endless times and when you read code you want to skim it. Not digest it.

massich · 2019-07-12T16:25:15Z

mne/source_tfr.py

+        self._update_times()
+        self.subject = _check_subject(None, subject, False)
+
+    def __repr__(self):  # noqa: D105


Don't use '%' to parse strings (we are no longer in the 90s ;) ).

fstrings are much more readable and they are the way to go (see doc). However, we cannot use them until we drop python 3.5 support. Meanwhile, we could be using 'my name is {name}'.format(name='massich')

https://realpython.com/python-f-strings/

We still use % all over the code base and have not agreed to change to / prefer .format -- I think % is fine for now

massich · 2019-07-12T16:26:40Z

mne/source_tfr.py

+        """
+        if ftype != 'h5':
+            raise ValueError('%s objects can only be written as HDF5 files.'
+                             % (self.__class__.__name__,))


massich · 2019-07-12T16:31:29Z

mne/source_tfr.py

+            raise ValueError('%s objects can only be written as HDF5 files.'
+                             % (self.__class__.__name__,))
+        if not fname.endswith('.h5'):
+            fname += '-stfr.h5'


avoid ifs without elses as much as possible. See this awesome talk by @ Katrina Owen

in a case like this one, I would do something like:

fname = fname if fname.endswith('h5') else '{}-stfr.h5'.format(fname)

or any way you like to build the fname. (I like this one because it has no spaces but maybe fname + '-stfr.h5' is a better choice)

massich · 2019-07-12T16:34:19Z

mne/source_tfr.py

+            return self._data.shape
+        return (self._kernel.shape[0],
+                self._sens_data.shape[1],
+                self._sens_data.shape[2])


use else!!!

document what returns.

does this return always the same kind of tuple?

massich · 2019-07-12T16:38:23Z

mne/source_tfr.py

+
+    @times.setter
+    def times(self, value):
+        raise ValueError('You cannot write to the .times attribute directly. '


why is this a value error? isn't it a runtime?

Is there any other way to prevent the setter? maybe not.

massich · 2019-07-12T16:38:45Z

mne/tests/test_source_tfr.py

+from mne.source_tfr import SourceTFR
+
+
+def _fake_stfr():


massich · 2019-07-12T16:38:51Z

mne/tests/test_source_tfr.py

+def _fake_kernel_stfr():
+    """Create a fake kernelized SourceTFR."""
+    kernel = np.random.rand(100, 40)
+    sens_data = np.random.rand(40, 20, 10)


massich · 2019-07-12T16:44:02Z

mne/tests/test_source_tfr.py

+
+    # check if data is in correct shape
+    expected = [100, 10, 30]
+    assert_allclose([kernel_stfr.shape, full_stfr.shape,


why are you testing it like this? by creating a vector that will have no name and when we read the error we will know nothing about it? why allclose? is it possible to have an array with shape 100+epsilon?

assert kernel_stfr.shape == (100, 10, 30) assert full_stfr.shape == (100, 10, 30) assert bla.shape == (100, 10, 30) assert foo.shape == (watever)

will return a meaningful error that I will read and I would understand what is my next step. otherwise, I'll have to put a breakpoint stop the execution, understand the array you have created think about all those things and make a guess of what could be wrong. I'm not smart enough. I would rather prefer pytest telling me where should I look.

massich · 2019-07-12T16:44:34Z

mne/tests/test_source_tfr.py

+    assert_equal(stfr._data.shape[-1], n_times)
+    assert_array_equal(stfr.times, stfr.tmin + np.arange(n_times) * stfr.tstep)
+
+    assert_array_almost_equal(


avoid almost_equal either equal or allclose

massich · 2019-07-12T16:47:05Z

mne/tests/test_source_tfr.py

+
+    # tstep <= 0 is not allowed
+    pytest.raises(ValueError, attempt_assignment, stfr, 'tstep', 0)
+    pytest.raises(ValueError, attempt_assignment, stfr, 'tstep', -1)


I know this is compact, but when I read this I need to go from
attempt_assignment, stfr, 'tstep', 0 to attempt_assignment(foo=foo, bar=bar..) inside my head. Its better to write with pytest.raises(ValueError, match='AND A MEANINGFUL ERROR MATCH THAT TELLS ME WHY THIS SHOULD FAIL')

massich · 2019-07-12T16:47:57Z

mne/tests/test_source_tfr.py

+    pytest.raises(ValueError, attempt_assignment, stfr_kernel, 'data',
+                  [np.arange(100)])
+    pytest.raises(ValueError, attempt_assignment, stfr_kernel, 'data',
+                  [[[np.arange(100)]]])


massich · 2019-07-12T16:48:19Z

mne/tests/test_source_tfr.py

+@requires_h5py
+def test_io_stfr_h5():
+    """Test IO for stfr files using HDF5."""
+    for stfr in [_fake_stfr(), _fake_kernel_stfr()]:


This screams parametrize

massich · 2019-07-12T16:48:36Z

mne/tests/test_source_tfr.py

+    stfr_ = _fake_stfr()
+    kernel_stfr_ = _fake_kernel_stfr()
+
+    for stfr in [stfr_, kernel_stfr_]:


so does this one

massich · 2019-07-12T16:50:09Z

mne/tests/test_source_tfr.py

+
+        copy_2 = inst.copy()
+        assert_allclose(copy_2.crop(tmin=0.2).data, inst.data[:, :, 2:])
+        assert_allclose(copy_2.times, inst.times[2:])


the reference you compare with is the later assert actual == expected. Same thing `assert_allclose(actual, expected)

massich · 2019-07-12T16:50:46Z

mne/tests/test_source_tfr.py

+
+def test_invalid_params():
+    """Test catching invalid parameters for SourceTFR."""
+    data = np.random.rand(40, 10, 20)


either you use a seed and then you compare against it or use np.empty.

massich · 2019-07-12T16:51:13Z

mne/tests/test_source_tfr.py

+        assert_allclose(copy_2.times, inst.times[2:])
+
+
+def test_invalid_params():


parametrize

...according to review in PR mne-tools#6543 Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

DiGyt · 2019-08-05T12:17:25Z

Thanks for the review @massich and all others!

I didn't implement the changes here, but in PR #6629 . As changes in # 6629 will highly depend on changes here (and vice versa), I think it's the best idea to maintain everything in one comprehensive pull and close this one.

massich · 2019-08-05T15:32:17Z

I didn't implement the changes here, but in PR #6629 . As changes in # 6629 will highly depend on changes here (and vice versa), I think it's the best idea to maintain everything in one comprehensive pull and close this one.

Usually we try to isolate the things, split them as much as possible and write dependencies between them. (makes review easier)

DiGyt added 5 commits June 6, 2019 14:36

Added SourceTFR

a65932f

Added Basic SourceTFR structure and Text Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Added SourceTFR

dcd19e9

Added Basic SourceTFR structure and Text Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Merge https://github.com/mne-tools/mne-python into SourceTFR

1802316

Updated SourceTFR

df8aa5a

Updated SourceTFR to version used for reading morlet/multitapers Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

Merge https://github.com/mne-tools/mne-python into SourceTFR

d8ead22

massich reviewed Jul 12, 2019

View reviewed changes

mne/tests/test_source_tfr.py

from mne.source_tfr import SourceTFR

def _fake_stfr():

Copy link
Copy Markdown

Contributor

massich Jul 12, 2019

Choose a reason for hiding this comment

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

fixture

massich reviewed Jul 12, 2019

View reviewed changes

mne/tests/test_source_tfr.py

assert_allclose(copy_2.times, inst.times[2:])

def test_invalid_params():

Copy link
Copy Markdown

Contributor

massich Jul 12, 2019

Choose a reason for hiding this comment

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

parametrize

DiGyt added a commit to DiGyt/mne-python that referenced this pull request Aug 5, 2019

cleaned up SourceTFR + SourceTFR tests...

56c83fa

...according to review in PR mne-tools#6543 Signed-off-by: Dirk Gütlin <dirk.guetlin@stud.sbg.ac.at>

DiGyt mentioned this pull request Aug 5, 2019

Time-frequency in Source Space #6629

Closed

DiGyt closed this Aug 5, 2019

		assert_allclose(copy_2.times, inst.times[2:])


		def test_invalid_params():

Uh oh!

Conversation

DiGyt commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

massich Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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!

DiGyt commented Aug 5, 2019

Uh oh!

massich commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

massich Jul 12, 2019 •

edited

Loading

massich Jul 12, 2019 •

edited

Loading

massich Jul 12, 2019 •

edited

Loading