MAINT: Refactor digitation for KIT, artemis123, bti by massich · Pull Request #6368 · mne-tools/mne-python

massich · 2019-05-23T11:54:20Z

This PR:

takes over [WIP] refactor to prepare digitization #5986, Add Digitization class #6362.
Refactors part of the digitization to make them look similar (for further refactor).

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

agramfort · 2019-05-23T12:06:46Z

looking good so far 👍

codecov · 2019-05-23T12:50:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b9b2c68). Click here to learn what that means.
The diff coverage is 93.25%.

@@            Coverage Diff            @@
##             master    #6368   +/-   ##
=========================================
  Coverage          ?   89.32%           
=========================================
  Files             ?      408           
  Lines             ?    74300           
  Branches          ?    12292           
=========================================
  Hits              ?    66371           
  Misses            ?     5072           
  Partials          ?     2857

larsoner · 2019-05-23T13:35:40Z

Let me know when CIs are green and I should look

massich · 2019-05-23T14:09:45Z

@larsoner I'm AFK but Travis is green

massich · 2019-05-23T14:10:26Z

But coverage went off. I'll check

massich · 2019-05-23T14:15:36Z

Oh yes, I did not update the doc so it searches mne.io.DigPoint rather than mne.digitization.DigPoint

massich · 2019-05-27T11:38:28Z

Can I get feedback for this PR?

cc: @mne-tools/contributors

agramfort

just a few nitpicks. nice work @massich

agramfort · 2019-05-27T19:00:29Z

mne/digitization/_utils.py

+
+
+def _read_dig_points(fname, comments='%', unit='auto'):
+    """Read digitizer data from a text file.


apparently you can read more than from a .txt file

agramfort · 2019-05-27T19:02:16Z

mne/digitization/_utils.py

+
+    Returns
+    -------
+    dig : Digitization


maybe you can already put a doc template for a parameter of type dig?

agramfort · 2019-05-27T19:04:57Z

mne/digitization/_utils.py

+
+    Parameters
+    ----------
+    mrk : None | str | array_like, shape = (5, 3)


can you remove the = after shape in this docstring?

agramfort · 2019-05-27T19:05:21Z

mne/digitization/_utils.py

+
+    Returns
+    -------
+    dig_points : list


use a standard docstring

what is not standard? in this return?

agramfort · 2019-05-27T19:05:55Z

mne/digitization/_utils.py

+    dig_points : list
+        List of digitizer points for info['dig'].
+    dev_head_t : dict
+        A dictionary describe the device-head transformation.


do we have a template docstring for this?

No I'll make a PR.

I'll push them in #6385

agramfort · 2019-05-27T19:06:28Z

mne/digitization/_utils.py

+        mrk = read_mrk(mrk)
+
+    # hsp = apply_trans(als_ras_trans, hsp)
+    # elp = apply_trans(als_ras_trans, elp)


it was there before? is this expected?

agramfort · 2019-05-27T19:07:07Z

mne/digitization/_utils.py

+# From bti
+def _make_bti_dig_points(nasion, lpa, rpa, hpi, extra,
+                         convert=False, use_hpi=False,
+                         bti_dev_t=False, dev_ctf_t=False):


I don't think is necessary this function should die in the next PR.

agramfort · 2019-05-27T19:08:00Z

mne/digitization/base.py

+    ident : int
+        Identifier.
+    r : ndarray, shape (3,)
+        Position.


position in meters?

massich · 2019-05-27T20:23:19Z

I did not touch the docstrings much. (Just a find and replace of dig: list of dicts to Digitization and did not revert). I'll push a pass.

agramfort · 2019-05-27T20:31:28Z

ok to merge then when you're happy and CIs are green. I agree to do one thing at a time

larsoner · 2019-05-27T22:03:33Z

I probably can't really look until Friday so don't wait for me.

GuillaumeFavelier

I have only small suggestions, thank you for your work @massich

GuillaumeFavelier · 2019-05-28T12:32:14Z

mne/digitization/_utils.py

+from ..transforms import Transform
+from ..transforms import combine_transforms
+from ..transforms import invert_transform
+from ..transforms import _to_const


You prefer one import at a time?

we don't have a strict policy that I'm aware of, and its easier like this when I'm moving code around. I'll do a clean up on the last PR.

GuillaumeFavelier · 2019-05-28T12:34:02Z

mne/digitization/_utils.py

+    elp = elp[3:]
+
+    dig_points = _make_dig_points(nasion, lpa, rpa, elp, hsp)
+    dev_head_t = Transform('meg', 'head', trans)


To be consistent with previous call:
dev_head_t = Transform(fro='meg', to='head', trans=trans)

agreed. I'll at them on the next PR.

GuillaumeFavelier · 2019-05-28T12:34:06Z

mne/digitization/_utils.py

+                               'meg', 'ctf_head')
+        dev_head_t = combine_transforms(t, ctf_head_t, 'meg', 'head')
+    else:
+        dev_head_t = Transform('meg', 'head', trans=None)


dev_head_t = Transform(fro='meg', to='head', trans=None)

mne/digitization/_utils.py

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

Joan Massich and others added 19 commits May 22, 2019 16:28

move Digpoint to digitization

368ffed

ENH: Add Digitization

1be3601

ENH: allow Digitization comparison

1dd80da

Create a helper class for MNE objects list

92e1f3f

move kit stuff

d87be64

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

fix: kitEpoch.__init__

28154aa

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

migrate artemis123

dd7d21c

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

start moving BTI (after BTI refactor)

4af597d

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

update the signature to use nasion, lpa, rpa, hpi, extra

0472dd6

unpack idx_points inside _read_head_file

8e5e4ef

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

ENH: Add Digitization class

eb4a71a

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

MNT: split _makedig_points

237fc49

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

remove redundant operation

fa1c20f

Fix: __delitem__

51ea4dc

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

pep

0c94c97

MAINT: remove io.meas_info duplicates in digitization._utils

73bd194

FIX: migrate _mess to _utils

9c21cfa

Remove class Digitization

1a45b31

clean-up

2e607d8

massich pushed a commit to massich/mne-python that referenced this pull request May 23, 2019

squash mne-tools#6368

00a9130

massich mentioned this pull request May 23, 2019

ENH: Add digitization class #6369

Merged

fix: import

36d371a

[skip travis][skip azp] fix circle

ca0af70

agramfort reviewed May 27, 2019

View reviewed changes

fix

afeb0ff

GuillaumeFavelier reviewed May 28, 2019

View reviewed changes

massich changed the title ~~Refactor digitation for KIT, artemis123, bti~~ MAINT: Refactor digitation for KIT, artemis123, bti May 29, 2019

massich merged commit 087a88e into mne-tools:master May 29, 2019

massich added a commit that referenced this pull request May 29, 2019

MAINT: Refactor digitation for KIT, artemis123, bti (#6368)

f4ff170

Co-authored-by: Teon Brooks <teon.brooks@gmail.com>

massich deleted the refactor_digitation branch May 29, 2019 13:24

massich mentioned this pull request Jun 17, 2019

Digitization progress #6461

Closed

19 tasks



		def _read_dig_points(fname, comments='%', unit='auto'):
		"""Read digitizer data from a text file.

Uh oh!

Conversation

massich commented May 23, 2019

Uh oh!

agramfort commented May 23, 2019

Uh oh!

codecov bot commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

larsoner commented May 23, 2019

Uh oh!

massich commented May 23, 2019

Uh oh!

massich commented May 23, 2019

Uh oh!

massich commented May 23, 2019

Uh oh!

massich commented May 27, 2019

Uh oh!

agramfort left a comment

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!

massich commented May 27, 2019

Uh oh!

agramfort commented May 27, 2019

Uh oh!

larsoner commented May 27, 2019

Uh oh!

GuillaumeFavelier left a comment

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 23, 2019 •

edited

Loading