-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
…hod` similar to how `ScatteredInterpolationMethod` currently exists.
Replaced by `NoShading`
This makes for cleaner GC/windups when deleting a plot - the lift observer functions are known to the plot object, and can be removed immediately instead of relying on the GC to detect that those variables are no longer in use.
Codecov ReportAttention: Patch coverage is
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. |
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? |
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 |
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) |
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.
Just some things I noticed about the multithreading options
This should be ready for review/merge now. I also added a commit to add Makie v0.21 compatibility. |
Just wanted to bump this - is there anything holding it back at this point? |
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.
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
Well, that was celebrated too early. Documentation is failling. @asinghvi17 can you have a look?
https://github.com/MakieOrg/TopoPlots.jl/actions/runs/9512297332/job/26220010961 |
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! |
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.