Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] fix elp reading #3690

Merged
merged 2 commits into from
Oct 25, 2016
Merged

Conversation

jona-sassenhagen
Copy link
Contributor

#3613 breaks .elp reading by doing a transpose at the wrong time. This fixes it.

@Eric89GXL
(sorry for the mess, I'm not used to having upstream push access ...)

Closes #3684

@@ -266,7 +266,7 @@ def read_montage(kind, ch_names=None, path=None, unit='m', transform=False):
az = np.deg2rad(np.array([90. - h if a >= 0. else -90. - h
for h, a in zip(horiz, az)]))
pol = radius * np.pi
pos = _sph_to_cart(np.array([np.ones(len(az)) * 85., az, pol])).T
pos = _sph_to_cart(np.array([np.ones(len(az)) * 85., az, pol]).T)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like the right change. Can you add a test? You're right that we need one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm surprised current tests didn't catch it ...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the array was 3x3, and the output values (which would have been very broken) never checked. Or maybe the lines were never tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think all of these channel position readers are tested at the same time, so if it's a problem with one, it's possibly a problem with all.

Copy link
Member

Choose a reason for hiding this comment

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

Can you look into it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call Eric:

        'EEG\t      F3\t -62.027\t -50.053\t      85\n'
        'EEG\t      Fz\t  45.608\t      90\t      85\n'
        'EEG\t      F4\t   62.01\t  50.103\t      85\n',

I'll go for it.

@jona-sassenhagen
Copy link
Contributor Author

@Eric89GXL WDYT? Maybe a bit messy.

@larsoner larsoner changed the title fix elp reading [MRG+1] fix elp reading Oct 24, 2016
@larsoner
Copy link
Member

LGTM, +1 for merge once the CIs come back happy

@codecov-io
Copy link

codecov-io commented Oct 24, 2016

Current coverage is 87.44% (diff: 100%)

Merging #3690 into master will increase coverage by 0.03%

@@             master      #3690   diff @@
==========================================
  Files           343        343          
  Lines         60888      60989   +101   
  Methods           0          0          
  Messages          0          0          
  Branches       9307       9355    +48   
==========================================
+ Hits          53220      53329   +109   
+ Misses         4912       4909     -3   
+ Partials       2756       2751     -5   

Sunburst

Powered by Codecov. Last update 24ca938...e88455b

@jona-sassenhagen
Copy link
Contributor Author

Can I get another review? :)

@agramfort agramfort merged commit 8ede39d into mne-tools:master Oct 25, 2016
@agramfort
Copy link
Member

thx @jona-sassenhagen

@jona-sassenhagen
Copy link
Contributor Author

Thanks guys

@jona-sassenhagen jona-sassenhagen deleted the fix_elp_reading branch October 25, 2016 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#3613 causes problems with loading .elps
4 participants