MakieOrg / TopoPlots.jl

Makie topo plot recipes, for neuro-science, geo plots and anyone needing surface plots from unstructured data
MIT License
26 stars 8 forks source link

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

Open marcpabst opened 9 months ago

marcpabst commented 9 months ago

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: https://github.com/MakieOrg/TopoPlots.jl/issues/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.

ararslan commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

behinger commented 9 months ago

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 https://github.com/MakieOrg/TopoPlots.jl/blob/7e511099803389545d69fb43daae5ac0c69d0046/src/extrapolation.jl#L9 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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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"