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

Improved interpolations #15

Closed
wants to merge 6 commits into from

Conversation

yakir12
Copy link

@yakir12 yakir12 commented Jul 26, 2022

This fixes the interpolation problems mentioned in #9.

The core of the problem is the introduction of zeros along the perimeter of the bounding geometry. Instead, the 4 corners of the bounding rectangle that contains all of the data (including the geometry) are added and set to pad_value (e.g. zero). When the geometry is a circle, this solves cases where a known data point is larger than zero AND close to the perimeter of the geometry. Because I refrain from setting the perimeter data to zero, using the DelaunayMesh is completely impossible. We would need to be able to interpolate such a mesh for that to work. Ideally, JuliaMath/Interpolations.jl#118
would be solved and we could simply use Julia's Interpolations.jl.

This PR is quite aggressive, so I expect some pushback and appreciate any feedback you may have.

@yakir12
Copy link
Author

yakir12 commented Jul 26, 2022

I'll go ahead and ping @behinger in case he's not on parental leave yet 😬 and @palday, maybe you could review this PR. Thanks!

@palday palday requested review from palday and behinger July 26, 2022 14:51
src/interpolators.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/core-recipe.jl Outdated Show resolved Hide resolved
@palday
Copy link
Collaborator

palday commented Jul 28, 2022

@yakir12 can you add in a test for peaks not shifting away from their nominal location? Te example using four points near the boundary was a good one.

@codecov-commenter
Copy link

Codecov Report

Merging #15 (253dbcd) into master (b2f8b98) will increase coverage by 4.16%.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   90.83%   95.00%   +4.16%     
==========================================
  Files           4        4              
  Lines         120      120              
==========================================
+ Hits          109      114       +5     
+ Misses         11        6       -5     
Impacted Files Coverage Δ
src/TopoPlots.jl 100.00% <ø> (ø)
src/interpolators.jl 100.00% <ø> (+6.25%) ⬆️
src/core-recipe.jl 92.95% <91.17%> (+6.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2f8b98...253dbcd. Read the comment docs.

@yakir12
Copy link
Author

yakir12 commented Aug 5, 2022

I'll keep this open in case you need it for now.

@behinger
Copy link
Collaborator

behinger commented Aug 5, 2022

pad_value prior could be an integer, now has to be a float. Not sure where this cropped up. Not a biggy, but took me a bit to debug

the results look much better
old

new

but, the interpolations still look quite different (albeit much better), compared to PyMNE

as a more comparable comparison (imho), this is with extrapolate="box"

MNE Plot via:

posMat = Matrix(hcat(pos...)).-0.5
# you can also make .*0.5 afterwards, then all points are within the head
	#pos = PyMNE.channels.make_eeg_layout(mon).pos
PyMNE.viz.plot_topomap(rand(MersenneTwister(1),length(pos)),posMat',cmap="RdBu_r")

@behinger
Copy link
Collaborator

behinger commented Aug 5, 2022

another point to discuss: does this work also for other bounding geometries? e.g. if you have a weird polynomial, you might want to deactivate this functionality? Or should this even be "off" by default, and eeg_topoplot activates it?

Answer: Just saw, that geometry should be just circle and box, did you check what happens in case of adding this to a box-type enclosure?

Besides this: I think it is a clear improvement and should be merged. Integration with #12 will help debug this further

@SimonDanisch SimonDanisch mentioned this pull request Aug 17, 2022
@palday
Copy link
Collaborator

palday commented Aug 30, 2022

Superseded by #20

@palday palday closed this Aug 30, 2022
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.

4 participants