ENH: Add digitization class #6369

massich · 2019-05-23T12:46:21Z

This PR:

This PR requries MAINT: Refactor digitation for KIT, artemis123, bti #6368
modifies _test_raw_reader to force info['dig'] to be Digitization or None
Add Digitization class

On the top, here is a summary of which readers go through _test_raw_reader, if so with was type(info['dig']): None, Digitization, or both. And for those readers expected to not have Digitization do they have montage. The goal of this table is to keep in mind the current status + plan following PRs.

                       | _test_raw_reader | None | Digitization | other   |
-----------------------+------------------+------+--------------+---------+
RawArray               |     x            |  x   |              |         |
read_raw_artemis123    |     x            |      |     x        |         |   
read_raw_bdf           |                  |      |              | montage |
read_raw_brainvision   |     x            |  x   |     x        |         |    
read_raw_bti           |     x            |  x   |     x        |         |
read_raw_cnt           |     x            |  x   |              | montage |
read_raw_ctf           |     x            |      |     x        |         |
read_raw_edf           |     x            |  x   |     x        |         |
read_raw_eeglab        |     x            |      |     x        |         |
read_raw_egi           |     x            |  x   |              | montage |
read_raw_eximia        |     x            |  x   |              | ------- |
read_raw_fieldtrip     |                  |      |              | ------- |
read_raw_fif           |     x            |      |     x        |         |
read_raw_gdf           |     x            |  x   |              | montage |
read_raw_kit           |     x            |  x   |     x        |         |
read_raw_nicolet       |     x            |  x   |              | montage |

lgtm-com · 2019-05-23T13:33:56Z

This pull request introduces 1 alert when merging 835f0e0 into b9b2c68 - view on LGTM.com

new alerts:

1 for Unreachable code

Comment posted by LGTM.com

codecov · 2019-05-27T09:59:00Z

Codecov Report

Merging #6369 into master will increase coverage by <.01%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           master    #6369      +/-   ##
==========================================
+ Coverage   89.26%   89.27%   +<.01%     
==========================================
  Files         410      411       +1     
  Lines       74435    74469      +34     
  Branches    12312    12316       +4     
==========================================
+ Hits        66446    66482      +36     
+ Misses       5135     5131       -4     
- Partials     2854     2856       +2

lgtm-com · 2019-05-27T10:42:36Z

This pull request introduces 1 alert when merging 7f37ecb into 5093d00 - view on LGTM.com

new alerts:

1 for Unreachable code

Comment posted by LGTM.com

massich · 2019-05-29T14:35:53Z

mne/utils/_bunch.py

+            self._items = list()
+        elif all([isinstance(_, kls) for _ in elements]):
+            if elements is None:
+                self._items = list()


move this on the previous condition.
if elements is None or (isinstance(elements, list) and not elements):

massich · 2019-05-29T14:37:08Z

mne/utils/_bunch.py

+                self._items = deepcopy(list(elements))
+        else:
+            # XXX: _msg should not be Digitization related
+            _msg = 'Digitization expected a iterable of DigPoint objects.'


this should be addressed. (the tests should be modified aswell)

larsoner · 2019-05-29T14:50:58Z

mne/io/meas_info.py

@@ -156,6 +158,7 @@ def _unique_channel_names(ch_names):


 # XXX Eventually this should be de-duplicated with the MNE-MATLAB stuff...
+# XXX: Digitization, Docstrings need a pass changing lists for Digitization


Is this still true?

~~The one I added? yes. I did not do the pass.~~ True the pass is not needed there but on the refactorized.

larsoner · 2019-05-29T14:55:14Z

mne/utils/_bunch.py


 class NamedFloat(_Named, float):
    """Float with a name in __repr__."""

-    pass
+
+class MNEObjectsList(MutableSequence):


One problem I expect with this subclassing MutableSequence instead of list is that mne.externals.h5io.write_hdf5(...) will not work on it. IIRC currently you can write_hdf5 on info objects and it will work because all info entries and Python built-in types. After this PR it will not. Can you check? If this is the case we should fix it somehow.

I would consider just subclassing list like we do for SourceSpaces, and if at some point we want to not subclass list for these sorts of things, we can change Digitization and SourceSpaces at the same time.

I have the same feeling as @larsoner here. Subclassing list is not a great pattern but it's simple. We just want to have a repr. The argument to make sure that it contains only instance of DigPoint is fair but do we really expect to have the problem with users hacking the info['dig'] manually? I don't expect anyone to do this and think that it will be fine...

my 2c

I can make MNEObjectsList derive from list() I can leave with it.

I am not expecting "users" to mess up the list, but I do like to have some safety nets when writing digitizations, montages, etc.. (or force developers of new readers to use these safeties) I can always get read of this class and do that in Digitization, or keep it as a private thing. But we have lots of lists of things and we never ensure those elements are always the same. (if I'm not mistaken, this object does not prevent insertions of other types. I thought YAGNI, and that it was easy to add if needed)

can we avoid the MNEObjectsList and have Digitization inherit from list? it would remove a lot of code in this PR

It's a trade off between safety net and barrier to write new code. The more you nest classes, the less accessible it becomes to new contributors. I tend to lean more on the side of assuming good intent -- i.e., if you hack info['dig'] you are on your own.

lgtm-com · 2019-05-29T15:13:13Z

This pull request introduces 1 alert when merging 8b17361 into f4ff170 - view on LGTM.com

new alerts:

1 for Unreachable code

Comment posted by LGTM.com

jasmainak · 2019-06-02T22:27:29Z

mne/digitization/base.py

@@ -73,3 +74,16 @@ def __eq__(self, other):  # noqa: D105
            return False
        else:
            return np.allclose(self['r'], other['r'])
+
+
+class Digitization(MNEObjectsList):


New public class has been added but it's not documented in API reference.

massich · 2019-06-03T10:22:03Z

Do we miss entry to the glossary? cc: @drammock
Shall we review the visualization module? cc: @GuillaumeFavelier

agramfort · 2019-06-03T11:38:33Z

mne/utils/numerics.py

@@ -663,13 +663,15 @@ def object_size(x):
    x : object
        Object to approximate the size of.
        Can be anything comprised of nested versions of:
-        {dict, list, tuple, ndarray, str, bytes, float, int, None}.
+        {dict, list, tuple, ndarray, str, bytes, float, int, None,
+        Digitization}.


maybe the changes in this file are not needed as Digitization is just a list?

great catch!

massich · 2019-06-03T16:17:59Z

This should be green. Feel free to merge.

larsoner · 2019-06-03T16:41:40Z

mne/digitization/_utils.py

@@ -261,7 +261,7 @@ def _make_dig_points(nasion=None, lpa=None, rpa=None, hpi=None,
                        'kind': FIFF.FIFFV_POINT_EEG,
                        'coord_frame': FIFF.FIFFV_COORD_HEAD})

-    return _format_dig_points(dig)
+    return Digitization(_format_dig_points(dig))


It looks like things would be a bit simpler if _format_dig_points returned a list cast to Digitization already. Then there wouldn't need to be a bunch of Digitization(_format_dig_points(...)) calls

Agreed, that was on my list already. But I can do it here. no prob.

I would have liked better to do it the other way around. Prefer to import a constructor and have a call that does something = MyConstructor() than importing a private function and call something = _my_obscure_func(foo).

But since I'll be doing subsequent PRs in these things, I'll change it back.

GuillaumeFavelier · 2019-06-03T17:48:13Z

@massich as we discussed we could review the visualization module. I think having a dig.plot() function could be a good idea for two reasons. For context encapsulation and from a user point of view, it would be easier to use. Also, something similar is already done for other classes (i.e. stc.plot() and evoked.plot()) so it's consistent with the architecture. If you want, we can discuss how to make it work with a potential follow-up PR, what do you think?

drammock · 2019-06-03T18:11:25Z

Do we miss entry to the glossary? cc: @drammock

Added "Digitization" to TODO list in #6321

massich · 2019-06-04T08:19:13Z

@GuillaumeFavelier great! we should open an issue and cross reference it here.

massich force-pushed the add_Digitization branch from 7f37ecb to 8b17361 Compare May 29, 2019 14:23

massich commented May 29, 2019

View reviewed changes

massich marked this pull request as ready for review May 29, 2019 14:39

massich requested review from agramfort and larsoner and removed request for agramfort May 29, 2019 14:40

larsoner reviewed May 29, 2019

View reviewed changes

jasmainak reviewed Jun 2, 2019

View reviewed changes

massich force-pushed the add_Digitization branch from 8b17361 to b4a37da Compare June 3, 2019 10:12

agramfort reviewed Jun 3, 2019

View reviewed changes

larsoner reviewed Jun 3, 2019

View reviewed changes

massich added 8 commits June 4, 2019 10:06

Add Digitization class

0ede889

FIX: prefer inherit from list

82a8ca4

FIX: Remove unreachable code

34ddade

ENH: keep pass

8a64347

DOC: add entry on the API

558cd2e

update whatsnew

00467fb

FIX: revert mne.utils.numerics to master

4c38ef3

_format_dig_points return Digitization object

7fae35a

massich force-pushed the add_Digitization branch from cd4f78c to 7fae35a Compare June 4, 2019 08:16

massich changed the title ~~Add digitization class~~ ENH: Add digitization class Jun 4, 2019

massich merged commit dbda584 into mne-tools:master Jun 4, 2019

massich deleted the add_Digitization branch June 4, 2019 16:33

GuillaumeFavelier mentioned this pull request Jun 5, 2019

VIZ: Add a plot method to the digitization class #6411

Closed

cbrnr mentioned this pull request Jun 6, 2019

Adding a reference does not work after a montage has been set #5619

Closed

massich mentioned this pull request Jun 17, 2019

Digitization progress #6461

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add digitization class #6369

ENH: Add digitization class #6369

massich commented May 23, 2019

lgtm-com bot commented May 23, 2019

codecov bot commented May 27, 2019 •

edited

Loading

lgtm-com bot commented May 27, 2019

massich May 29, 2019

massich May 29, 2019 •

edited

Loading

larsoner May 29, 2019

massich May 29, 2019 •

edited

Loading

larsoner May 29, 2019

agramfort May 29, 2019

massich May 29, 2019 •

edited

Loading

agramfort May 29, 2019

jasmainak Jun 2, 2019

lgtm-com bot commented May 29, 2019

jasmainak Jun 2, 2019

massich commented Jun 3, 2019

agramfort Jun 3, 2019

massich Jun 3, 2019

massich commented Jun 3, 2019

larsoner Jun 3, 2019

agramfort Jun 4, 2019

massich Jun 4, 2019

massich Jun 4, 2019

GuillaumeFavelier commented Jun 3, 2019

drammock commented Jun 3, 2019

massich commented Jun 4, 2019

		@@ -156,6 +158,7 @@ def _unique_channel_names(ch_names):


		# XXX Eventually this should be de-duplicated with the MNE-MATLAB stuff...
		# XXX: Digitization, Docstrings need a pass changing lists for Digitization

ENH: Add digitization class #6369

ENH: Add digitization class #6369

Conversation

massich commented May 23, 2019

lgtm-com bot commented May 23, 2019

codecov bot commented May 27, 2019 • edited Loading

Codecov Report

lgtm-com bot commented May 27, 2019

Choose a reason for hiding this comment

massich May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massich May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massich May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 29, 2019

Choose a reason for hiding this comment

massich commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massich commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeFavelier commented Jun 3, 2019

drammock commented Jun 3, 2019

massich commented Jun 4, 2019

codecov bot commented May 27, 2019 •

edited

Loading

massich May 29, 2019 •

edited

Loading

massich May 29, 2019 •

edited

Loading

massich May 29, 2019 •

edited

Loading