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

Plotting broken for subset of chanels (and no labels) #38

Closed
marcpabst opened this issue Jan 17, 2024 · 12 comments
Closed

Plotting broken for subset of chanels (and no labels) #38

marcpabst opened this issue Jan 17, 2024 · 12 comments

Comments

@marcpabst
Copy link

marcpabst commented Jan 17, 2024

Has anyone used this recently? I'm pretty sure it used to work at some point, but now it seems quite broken:

using TopoPlots
labels = ["cz", "fp1", "fp2", "fz"]
TopoPlots.eeg_topoplot(ones(length(labels)), labels; label_text=true, label_scatter=(markersize=10, strokewidth=2, strokecolor=:white))

... does neither prodcue correct sensor locations (look at Cz) nor does it plot any text for me:

image

I believe this is related to: #18, so it seems to be an unfortunate consequence of some design decisions. Nevertheless, I think this warrants fixing as the current behavior is extremely surprising.

@marcpabst marcpabst changed the title Plotting broken for subset of chanels (and not labels) Plotting broken for subset of chanels (and no labels) Jan 17, 2024
@ararslan
Copy link

Can you provide your versions of Julia, Makie, and TopoPlots? The Julia version will be in the REPL header and the VERSION constant, and the package versions can be found using ]status Makie TopoPlots.

@marcpabst
Copy link
Author

Sorry, forgot to add this! I'm using Maki Makie v0.20.4 and TopoPlots v0.1.6.

Julia Version 1.10.0
Commit 3120989f39b (2023-12-25 18:01 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 12 × Apple M2 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, apple-m1)
  Threads: 8 on 8 virtual cores
Environment:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 6

@palday
Copy link
Collaborator

palday commented Jan 17, 2024

what happens if you use Makie 0.19 / CairoMakie 0.10? I've encountered some weird behavior around recipes in Makie 0.20 and Topoplots is almost entirely implemented as recipes.

@marcpabst
Copy link
Author

Same result with Makie v0.19 and GLMakie v0.9.5

@behinger
Copy link
Collaborator

behinger commented Jan 17, 2024

Thanks a lot for the report!!

your code example didnt run on my machine at first

Failed to show value:

Can't interpolate in a range where cmin == cmax. This can happen, for example, if a colorrange is set automatically but there's only one unique value present.

which makes sense, if one inputs ones(3) - if I change it to 1:3 it works
Unbenannt


but we are running in a normalization issue I think I stumbled upon in a very different context. That is, in

middle = mean(positions)
we center the enclosing geometry around our positions. Thus if the head is not sampled nicely, we will get a terrible center, and the topoplot will be "wrong" (or in other words, the headshape will be centered not around "0.5/0.5" but on the center of the positions.

Simple fix could be to expose the "center" to be manually overwritable by eeg_topoplot to be 0.5/0.5


in any case, yet another bug, in order for labels to show up, we need to set labels=labels as a kwargs like this:
TopoPlots.eeg_topoplot(1:length(labels),labels;labels=labels,label_text = true,label_scatter=(markersize=10, strokewidth=2, strokecolor=:white))

Unbenannt

I never used the labels feature because it is so limited to 10_20, therefore I don't have too much experience with it.


@marcpabst
Copy link
Author

marcpabst commented Jan 17, 2024

Thanks a lot for the report!!

your code example didnt run on my machine at first

Weird, it runs for me - are you using an older Julia version?

we center the enclosing geometry around our positions. Thus if the head is not sampled nicely, we will get a terrible center, and the topoplot will be "wrong" (or in other words, the headshape will be centered not around "0.5/0.5" but on the center of the positions.

I there a reason for that? I would expect that points are passed in some sort of normalised coordinate system relative to the head...

in any case, yet another bug, in order for labels to show up, we need to set labels=labels as a kwargs like this: TopoPlots.eeg_topoplot(1:length(labels),labels;labels=labels,label_text = true,label_scatter=(markersize=10, strokewidth=2, strokecolor=:white))
I never used the labels feature because it is so limited to 10_20, therefore I don't have too much experience with it.

Passing `labels' as a krwarg works, thanks! Not sure if this is documented anywhere?

@behinger
Copy link
Collaborator

behinger commented Jan 18, 2024

regarding the first issue:
adding these two lines fixes it, as we define the geometry of interpolation ourselves:

bounding_geometry=Circle(Point2f(0.5,0.5), 0.5),
extrapolation = GeomExtrapolation(enlarge=5),) # only needed if you want extrapolation to the boundary

Unbenannt

Next I was confused because Cz is not exactly at 0.5, but if you read in the channel locations, it is at [0.45768744581851517
0.5165548695126896] - but the locations have been generated using MNE-python...

For me Cz (and in all MNE python plots) should be centered on the head, but for some reason it is not when generating (regenerated them just to be 100% sure)


General question to discuss By default, do we want to center positions for EEG plots, or do we expect some kind of 0.5/0.5 center?

The above "solution" is too verbose for users. But there are no standards in montage coordinat systems either... so either users have to transform their position, or we try to transform them for them...

@behinger
Copy link
Collaborator

ok, so I have been digging through the MNE code, it is quite tricky, with so many special cases. But what I found is that they also normalize their coordinates.

One issue with our generated layout => position data right now, is that we make use of their normalization which is a bit strange/undocumented, adds padding, etc. This is why Cz is not at 0.5 in our generated data.

I could regenerate data where Cz is at 0.5 if this is something that people think is more helpful.
This doesnt resolve the issue of scaling, but given that MNE does it as well, maybe we should simply document that this is happening, and show people how to specify their own bounding_geometry if they want something else?

@marcpabst
Copy link
Author

I generally think MNE compatibility is really important as most people probbaly don't use a pure Julia pipeline (so inputs are coming from either EEGLAB or MNE), so doing what they do sounds reasonable. I personally don't really care what normalised coordinate system is used (altough using either nasion, inion, or Cz as the origin feels right) as long as it's documented properly.

@behinger
Copy link
Collaborator

behinger commented Jan 22, 2024

I agree. But you never interact with the positions directly (in MNE), so I don't think in this particular case it is useful to follow MNE and not have Cz at 0.5/0.5.

But indeed, we should keep the normalization to have similar behavior. And then update the documentation with the above geometry "fix"

palday added a commit that referenced this issue Nov 16, 2024
* fix labels, fix #39

* added a backward compatability with warning

* removed NaturalNeighbours for now until #55 is fixed

* added example of contourf and surface to docs

* added an example with custom bounds #38

* merge fixes

* typo

* remove extra file

* minor formatting

* oops

* re-enable natural neighbours

---------

Co-authored-by: Phillip Alday <[email protected]>
@palday
Copy link
Collaborator

palday commented Nov 16, 2024

I think that #54 and #57 address this

@palday
Copy link
Collaborator

palday commented Nov 16, 2024

Feel free to re-open if things are still a problem after we release 0.2 in a few days!

@palday palday closed this as completed Nov 16, 2024
palday added a commit that referenced this issue Nov 16, 2024
* fix labels, fix #39

* added a backward compatability with warning

* removed NaturalNeighbours for now until #55 is fixed

* added example of contourf and surface to docs

* added an example with custom bounds #38

* added 10-05 template electrodes; breaking: new default enlarge = 1.0 for eeg-topoplot only

* renamed     T3 is now T7
    T4 is now T8
    T5 is now P7
    T6 is now P8
according to the MCN system

* fixed 10/20 old labels work now too

* fixed bug

* added test for 10-20/10-05

* fix indent

* formatting

* s/begin/let

* docstrings

* s/warn/warning

* docs fix

* again

* 🤦

* I give up

---------

Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
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

No branches or pull requests

4 participants