[WIP] refactor to prepare digitization by massich · Pull Request #5986 · mne-tools/mne-python

massich · 2019-02-22T17:55:34Z

The idea of this PR was to move all the code that seems to be dealing with digitization in a single place before start changing code and factoring similar functions.

But I get in dependency hell.

Any help is wellcome. @agramfort @larsoner

massich · 2019-02-22T19:06:14Z

Maybe I should propose a different refractor where io stays in io. And at some point we can get rid of having our structures inside io

larsoner · 2019-02-22T19:19:54Z

This pull request introduces 11 alerts when merging 32f17c8 into 3d12b2b - view on LGTM.com

new alerts:

6 for Use of the return value of a procedure
3 for Unused import
1 for Wrong name for an argument in a call
1 for Wrong number of arguments in a call

Comment posted by LGTM.com

larsoner · 2019-02-22T19:22:22Z

That would probably be easier. Moving things out of io makes problems harder

agramfort · 2019-02-22T21:09:05Z

I agree with @larsoner I would keep digitization.py in mne/io to avoid circular import mess

codecov · 2019-03-04T10:03:42Z

Codecov Report

Merging #5986 into master will decrease coverage by 0.29%.
The diff coverage is 61.83%.

@@            Coverage Diff            @@
##           master    #5986     +/-   ##
=========================================
- Coverage   89.17%   88.87%   -0.3%     
=========================================
  Files         416      422      +6     
  Lines       74933    75149    +216     
  Branches    12352    12392     +40     
=========================================
- Hits        66823    66792     -31     
- Misses       5226     5462    +236     
- Partials     2884     2895     +11

larsoner · 2019-03-04T11:58:39Z

This pull request introduces 4 alerts when merging 6c4423e into ce50c2b - view on LGTM.com

new alerts:

4 for Unused import

Comment posted by LGTM.com

larsoner · 2019-04-15T14:49:55Z

This pull request introduces 1 alert when merging b8218bb into 4555011 - view on LGTM.com

new alerts:

1 for Unused import

Comment posted by LGTM.com

larsoner · 2019-04-19T15:43:46Z

This pull request introduces 3 alerts when merging 50e46f0 into b3dfd7a - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unused import
1 for Unreachable code

Comment posted by LGTM.com

massich · 2019-04-19T16:01:03Z

At d1eaa8d, Digitization is a custom list of DigPoint all boilerplate.

@larsoner do you think that its worth to factor it out? I would like to be able to write something like (if I understand how Bunch works):

from ..utils import Bunch
from ..utils import MyListOfBunch

class DigPoint(Bunch):

    def __repr__(self):  # noqa: D105
        # maybe we need to write this one

    def __eq__(self, other):
        # this one should come for free


class Digitization(MyListOfBunch, Digpoint):
    """Represent a list of DigPoint objects.

    Parameters
    ----------
    elements : list
        A list of DigPoint objects.
    """
    pass # because everything comes for free

larsoner · 2019-04-19T17:13:59Z

This pull request introduces 3 alerts when merging 9fe020b into b3dfd7a - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unused import
1 for Unreachable code

Comment posted by LGTM.com

katarinaslama · 2019-04-19T22:59:08Z

The travis error seems to not be specific to this PR. (When I run make pep on the current mne dev branch, I get the same error.) Is there anything I could help with here?

larsoner · 2019-04-19T23:46:48Z

I don't get errors with make pep on master. The tracebacks I see on travis have to do with mne/digitization which is new. @katarinaslama can you paste the master error here (or open a PR to fix it)?

It's possible one of the PEP checkers updated and got more strict

katarinaslama · 2019-04-20T05:41:51Z

I don't get errors with make pep on master. The tracebacks I see on travis have to do with mne/digitization which is new. @katarinaslama can you paste the master error here (or open a PR to fix it)?

It's possible one of the PEP checkers updated and got more strict

It's also possible that I'm missing something. My read of the travis output suggests that lines 3386-3391 are the problem.

When I run make pep on master, I get the same output as line 3390:
make: *** [pep] Error 2

Am I misreading this? Thanks!

mne/digitization/base.py

larsoner · 2019-04-20T12:20:05Z

The failure it's much earlier in line 3324. make flake (which is called by make pep) fails on this PR, but it doesn't get reported until all pep targets are run, so you have to look father up to see it

larsoner · 2019-04-23T11:10:59Z

This pull request introduces 2 alerts when merging ce3c3ef into 9cc5d14 - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

larsoner · 2019-04-24T11:30:43Z

This pull request introduces 2 alerts when merging 890a232 into 92f61fa - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

larsoner · 2019-04-24T12:53:54Z

This pull request introduces 2 alerts when merging ea07437 into 92f61fa - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

massich · 2019-04-24T12:58:37Z

larsoner · 2019-04-24T13:08:42Z

Excellent, thanks for figuring this out

larsoner · 2019-04-24T15:00:40Z

This pull request introduces 2 alerts when merging b5c5504 into 2dd7aa0 - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

larsoner · 2019-04-26T14:05:05Z

This pull request introduces 2 alerts when merging 3fcf2b5 into c4a83c5 - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

larsoner · 2019-04-26T15:54:04Z

This pull request introduces 2 alerts when merging 577900c into b9dcbe1 - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

larsoner · 2019-04-26T16:44:37Z

This pull request introduces 2 alerts when merging 3fcf2b5 into 03bc75e - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

lgtm-com · 2019-05-02T11:31:31Z

This pull request introduces 2 alerts when merging 662dd13 into 3cc76b9 - view on LGTM.com

new alerts:

1 for Special method has incorrect signature
1 for Unreachable code

Comment posted by LGTM.com

codecov · 2019-05-02T14:52:17Z

Codecov Report

Merging #5986 into master will decrease coverage by 0.23%.
The diff coverage is 63.47%.

@@            Coverage Diff             @@
##           master    #5986      +/-   ##
==========================================
- Coverage   89.31%   89.07%   -0.24%     
==========================================
  Files         405      422      +17     
  Lines       74266    75143     +877     
  Branches    12297    12388      +91     
==========================================
+ Hits        66330    66933     +603     
- Misses       5072     5329     +257     
- Partials     2864     2881      +17

lgtm-com · 2019-05-02T15:34:44Z

This pull request introduces 2 alerts when merging e16c5af into 3cc76b9 - view on LGTM.com

new alerts:

1 for Unused import
1 for Unreachable code

Comment posted by LGTM.com

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

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

This pull request introduces 2 alerts when merging 51ea4dc into 5803738 - view on LGTM.com

new alerts:

1 for Unused import
1 for Unreachable code

Comment posted by LGTM.com

massich changed the title ~~refactor to prepare digitization~~ [WIP] refactor to prepare digitization Feb 22, 2019

massich force-pushed the refactor_digitation branch from 32f17c8 to 29c6c9f Compare March 4, 2019 10:03

massich force-pushed the refactor_digitation branch from 29c6c9f to 6c4423e Compare March 4, 2019 10:20

massich force-pushed the refactor_digitation branch from 6c4423e to 5c64b68 Compare March 21, 2019 11:01

massich force-pushed the refactor_digitation branch from d09c86b to b8218bb Compare April 15, 2019 14:10

massich force-pushed the refactor_digitation branch from b8218bb to 50e46f0 Compare April 19, 2019 15:03

agramfort reviewed Apr 20, 2019

View reviewed changes

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

massich force-pushed the refactor_digitation branch from 9fe020b to ce3c3ef Compare April 23, 2019 10:26

massich force-pushed the refactor_digitation branch from 890a232 to ea07437 Compare April 24, 2019 12:13

massich force-pushed the refactor_digitation branch from ea07437 to b5c5504 Compare April 24, 2019 14:20

massich force-pushed the refactor_digitation branch from 577900c to 3fcf2b5 Compare April 26, 2019 16:04

massich force-pushed the refactor_digitation branch 3 times, most recently from be646e3 to 662dd13 Compare May 2, 2019 10:47

Joan Massich and others added 14 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>

massich force-pushed the refactor_digitation branch from e16c5af to 51ea4dc Compare May 22, 2019 14:29

pep

0c94c97

massich closed this May 22, 2019

This was referenced May 22, 2019

Add Digitization class #6362

Closed

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

Merged

Uh oh!

Conversation

massich commented Feb 22, 2019

Uh oh!

massich commented Feb 22, 2019

Uh oh!

larsoner commented Feb 22, 2019

Uh oh!

larsoner commented Feb 22, 2019

Uh oh!

agramfort commented Feb 22, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

larsoner commented Mar 4, 2019

Uh oh!

larsoner commented Apr 15, 2019

Uh oh!

larsoner commented Apr 19, 2019

Uh oh!

massich commented Apr 19, 2019

Uh oh!

larsoner commented Apr 19, 2019

Uh oh!

katarinaslama commented Apr 19, 2019

Uh oh!

larsoner commented Apr 19, 2019

Uh oh!

katarinaslama commented Apr 20, 2019

Uh oh!

Uh oh!

larsoner commented Apr 20, 2019

Uh oh!

larsoner commented Apr 23, 2019

Uh oh!

larsoner commented Apr 24, 2019

Uh oh!

larsoner commented Apr 24, 2019

Uh oh!

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

Uh oh!

Uh oh!

larsoner commented Apr 24, 2019

Uh oh!

larsoner commented Apr 24, 2019

Uh oh!

larsoner commented Apr 26, 2019

Uh oh!

larsoner commented Apr 26, 2019

Uh oh!

larsoner commented Apr 26, 2019

Uh oh!

lgtm-com bot commented May 2, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

lgtm-com bot commented May 2, 2019

Uh oh!

lgtm-com bot commented May 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 4, 2019 •

edited

Loading

massich commented Apr 24, 2019 •

edited

Loading

codecov bot commented May 2, 2019 •

edited

Loading