kdtree icon indicating copy to clipboard operation
kdtree copied to clipboard

Heavy refactoring suggestion

Open Hiyorimi opened this issue 6 years ago • 5 comments

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.

Hiyorimi avatar Feb 07 '19 14:02 Hiyorimi

Codecov Report

Merging #2 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          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.

codecov-io avatar Feb 07 '19 15:02 codecov-io

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 =)

kyroy avatar Feb 11 '19 08:02 kyroy

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 avatar Feb 11 '19 10:02 Hiyorimi

@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.

kyroy avatar Apr 19 '20 11:04 kyroy

@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 :)

Hiyorimi avatar Apr 23 '20 09:04 Hiyorimi