kdtree
kdtree copied to clipboard
Heavy refactoring suggestion
Hey! Thank you for a great KDTree implementation. However, I had to alter some code to ease support of KDTrees in my project, so I suggest this refactoring, which you might (or might as well not) find useful.
I had to give up explicit points and range modules, to lessen number of imports. I also removed example classes to the _test files, so they are not imported with the main code.
Thank you for considering this pull request.
Codecov Report
Merging #2 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #2 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 5 2 -3
Lines 224 187 -37
=====================================
- Hits 224 187 -37
Impacted Files | Coverage Δ | |
---|---|---|
kdtree.go | 100% <100%> (ø) |
:arrow_up: |
range.go | 100% <100%> (ø) |
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 f5d74e8...5d4a72a. Read the comment docs.
Hi @Hiyorimi,
Thanks for the pull request. Could you please give some example code specifying your problem? I don't know if "lessen number of imports" is a good reason for changing the api heavily.
I see that you added Distance
and PlaneDistance
to the interface. I can see that this might be useful. But I would rather make them optional "overwritable" functions to keep the library easy to use.
I consider implementing this =)
I used Distance
because I use Haversine distance in my project. Some might use spherical distance, and that (in my opinion) should be an option.
Extra modules, which are used only in tests, are redundant in application aiming to deal with high load, and it is also keeps package more simple. Besides, these point will be anyway overwritten.
@Hiyorimi I would really like to enable custom distance functions. It would be great to have a good example to be able to test the changes. Do you have one for me? I already have a pretty good implementation draft. The example should include the distance and plane distance function.
@Hiyorimi I would really like to enable custom distance functions. It would be great to have a good example to be able to test the changes. Do you have one for me? I already have a pretty good implementation draft. The example should include the distance and plane distance function.
I didn't change anything nor got back to the project since I submitted PR :)