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

Implement interpolation methods from NaturalNeighbours.jl #46

Merged
merged 22 commits into from
Jun 14, 2024

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Apr 23, 2024

NaturalNeighbours.jl is a package which does Delaunay triangulation based interpolation on non-gridded data. This PR integrates it into TopoPlots via a direct dependency and an exposed NaturalNeighboursMethod.

The PR also introduces a reference page for interpolations in interpolation_reference.md, which currently just contains a bunch of plots for each interpolation object I could think of. The actual NaturalNeighbours interpolators are in the bottom two rows.

iTerm2 iH3KtW

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 43.47826% with 13 lines in your changes missing coverage. Please review.

Project coverage is 79.32%. Comparing base (db4c6b0) to head (4cd374d).
Report is 4 commits behind head on master.

Files Patch % Lines
src/interpolators.jl 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   85.54%   79.32%   -6.22%     
==========================================
  Files           5        5              
  Lines         166      179      +13     
==========================================
  Hits          142      142              
- Misses         24       37      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asinghvi17
Copy link
Member Author

asinghvi17 commented Apr 23, 2024

I'm not sure how to test this here, it looks like there's some kind of a reference testing setup though? Or should I just test that this works given that it's coming from an external and well-tested package?

@asinghvi17 asinghvi17 changed the title Implement regridding methods from NaturalNeighbours.jl Implement interpolation methods from NaturalNeighbours.jl Apr 23, 2024
@behinger
Copy link
Collaborator

this looks like a great PR!

Regarding the testing: We can test the CloughTocher against the reference python/scipy-MNE implementation. I'm not sure how I'd formally test the other implementations. For me the plots look pretty good, and I always used that as a sanity check.

One thing that would be great, is to write two sentences on what Interpolator we recommend in what usecase. That is, what is your motivation to include NaturalNeighbours in addition to the methods from ScatteredInterpolation?

@asinghvi17
Copy link
Member Author

This should now be in a mergeable state, but will need to wait to merge until NaturalNeighbours.jl is compatible with DelaunayTriangulation.jl v1.0 (cc @DanielVandH)

Copy link

@DanielVandH DanielVandH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some things I noticed about the multithreading options

src/interpolators.jl Outdated Show resolved Hide resolved
src/interpolators.jl Outdated Show resolved Hide resolved
@asinghvi17
Copy link
Member Author

This should be ready for review/merge now. I also added a commit to add Makie v0.21 compatibility.

@asinghvi17
Copy link
Member Author

Just wanted to bump this - is there anything holding it back at this point?

Copy link
Collaborator

@behinger behinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

PS: Holding back I think is my goldfish-memory and the unclear "decision-making" process in TopoPlots.jl, e.g. who is leading/deciding anyway here.

PPS: We could do the version bump directly here

@behinger behinger merged commit 7f81e8d into MakieOrg:master Jun 14, 2024
2 of 4 checks passed
@behinger
Copy link
Collaborator

behinger commented Jun 14, 2024

200w

Thanks for the contribution!!

@behinger
Copy link
Collaborator

Well, that was celebrated too early. Documentation is failling. @asinghvi17 can you have a look?

  exception = 
    The required storage space exceeds the available storage space:
    nxest or nyest too small, or s too small. Try increasing s.

https://github.com/MakieOrg/TopoPlots.jl/actions/runs/9512297332/job/26220010961

@asinghvi17
Copy link
Member Author

Ah, I guess that example is pretty fragile with random data. I will try to PR something that works more consistently, but I think if you rerun the documentation job a couple of times it should work!

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