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

BUG: fix elp montage rotation #3755

Closed

Conversation

jona-sassenhagen
Copy link
Contributor

With .elp channel locations, without this patch:

unknown-75

with this patch:

unknown-8

Possibly related to #3613 and/or #3690 ?

@Eric89GXL can you check? I'm really not sure.

@jona-sassenhagen
Copy link
Contributor Author

======================================================================
FAIL: Test topo to sphere conversion.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\conda\envs\test\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "C:\projects\mne-python\mne\tests\test_transforms.py", line 222, in test_topo_to_sph
    assert_allclose(pos, new[ii], atol=1e-7)
  File "C:\conda\envs\test\lib\site-packages\numpy\testing\utils.py", line 1297, in assert_allclose
    verbose=verbose, header=header)
  File "C:\conda\envs\test\lib\site-packages\numpy\testing\utils.py", line 665, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Not equal to tolerance rtol=1e-07, atol=1e-07
(mismatch 100.0%)
 x: array([ 0.353553,  0.612372,  0.707107])
 y: array([-0.612372, -0.353553,  0.707107])
>>  raise AssertionError('\nNot equal to tolerance rtol=1e-07, atol=1e-07\n\n(mismatch 100.0%)\n x: array([ 0.353553,  0.612372,  0.707107])\n y: array([-0.612372, -0.353553,  0.707107])')

Not sure what is going on.
@wmvanvliet maybe you have an idea?

@teonbrooks
Copy link
Member

I agree that the fix makes the plots look correct but the original equations seem correct. do you know what's going on here? is the .elp coordinate system like left-handed?

@larsoner
Copy link
Member

@jona-sassenhagen can you check with git bisect where it went wrong? I'm guessing it was one of my PRs... it would be good to beef up the tests more to make sure things get translated properly. If you don't have time let me know and I can look into it since I probably broke it.

@agramfort
Copy link
Member

FYI I've seen this type of rotations using eeglab .set files

cc @jmontoyam

@jasmainak
Copy link
Member

yeah, the fix doesn't look correct to me. Where did you get this .elp file from? Possible to add a test?

@mmagnuski
Copy link
Member

I've seen this types of rotations when converting eeglab files to fieldtrip (eeglab2fieltrip IIRC, but that was some time ago - that might have been fixed there already. eeglab and fieldtrip topoplots were giving different head rotations at that time [at least for EGI channel locs])

@agramfort
Copy link
Member

agramfort commented Nov 10, 2016 via email

@jona-sassenhagen
Copy link
Contributor Author

The error occurs with multiple Brainvision files and .elp files. These specific files gave correct results with previous versions of MNE.

I will try to find what version is to blame.

@jona-sassenhagen
Copy link
Contributor Author

I can't really pinpoint the error because of the error introduced in #3613 and fixed in #3690, because the error specifically affects .elp reading. It is possible I introduced this specific error in my fix in #3690.

The last commit before #3613 works (2fa35d4).
Then #3613 makes .elps unreadable.
From #3690 on, .elps can be read, but they are rotated (when I did #3690, I only checked central topos that didn't change much under rotation :().

This also indicates IMO that the test modified in #3690 may be insufficient ... as it relies too much on shapes rather than values of the returned arrays.

@Eric89GXL can you indeed take a look? #3613 is very long and I'm really bad with transforms ...

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.

6 participants