-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MRG, BUG: Use auto extrap and mean borders #7603
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
Conversation
|
Examples have rendered in case anyone wants to look |
|
Okay the MEG looks reasonable, but @mmagnuski you are right that the local convex hull is not always good -- for EEG in This makes me wonder if we want to add |
|
The renderings in this example look good to me. The renderings in this example also look good as far as the "local"/convex hull question goes, but it looks like the head outline is slightly off-center left-to-right? I realize that's not what you're addressing in this PR, but it's rather puzzling since both examples use the sample data. |
|
MEG topos look much nicer: but our EEG friends may have some concerns. extrapolate='auto' is maybe a good option. @mmagnuski @cbrnr thoughts @sappelhoff thoughts? |
|
it's still a bit unclear to me why the EEG sensors on sample are so
centered inside the head circle. There
are electrodes closer to the ears in 3d.
… |
Codecov Report
@@ Coverage Diff @@
## master #7603 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03%
==========================================
Files 454 454
Lines 83929 83759 -170
Branches 13324 13255 -69
==========================================
- Hits 75661 75483 -178
- Misses 5406 5410 +4
- Partials 2862 2866 +4 |
This will not work for the case linked above -- you have a (correct) hexagonal convex hull, so adding an additional circular mask on top of this will either be a no-op or it will remove some sensors. If we want to keep
Ahh yes, we'll need to tweak how far out we put the extra points, how many of them we have, etc. I changed some values here that might have made the "jagginess" problem worse, but we'll have to evaluate that in the context of not introducing "blobs", too. Ultimately if someone uses a low res, though, I don't mind them ending up with jagged lines...
Where do you see this? My understanding is that "head" really means "minimum of the head circle and the circle that encompasses all sensors", so it should always give at least a somewhat sensible one. It seems like the simplest solution is to make the default |
+1 on that one |
|
what do we do here to move on? |
|
My vote is to do |
|
that works for me !
… |
Okay done now along with |
|
I have not looked at all examples, but this looks okay at least. If others agree I can take a look at all examples: |
|
Looks ok. So, in a subsequent PR we can change the behavior of extrapolate='head' so that extrapolation points are not placed on the head outline if some channels extend beyond this outline, right? |
|
It already should extend outside the head, which is what you wanted, right? |
|
Ah, you changed it? Sorry, didn't notice. |
| use_radius = head_radius * 1.1 | ||
| points_x = np.cos(points_l) * use_radius + x | ||
| points_y = np.sin(points_l) * use_radius + y | ||
| use_radii = radii * 1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so radii are x and y clip radius now, not the head_radius, 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed!
|
My ICA components example looks good! |
agramfort
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if @cbrnr is happy I am happy :)
and MEG topos are gorgeous now !
|
Had a quick look at some examples, looks good to me as well 👍 Could you post your example once more with the new settings @cbrnr ? |
|
@larsoner |
|
Looks great, awesome work! |
|
@mmagnuski changed in 15e5bae#diff-c0cc0663d36fcb3b69d6e33a22573db1R87-R90 to: Okay? |
|
Perfect! |
|
Okay, hopefully it's easier to fix the remaining issues now! |
|
Thanks @larsoner! |




Probably a better solution than #7602.
WIP because:
Closes #7601
Closes #7602