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

Heavy refactoring suggestion #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Hiyorimi
Copy link

@Hiyorimi Hiyorimi commented Feb 7, 2019

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.

…hich copied from hongshibao/go-kdtree approach
…kages, ex-points code moved to _test files to let final binary build be lighter, some code formatting for the tests.
@codecov-io
Copy link

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%> (ø) ⬆️
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.

@Hiyorimi Hiyorimi mentioned this pull request Feb 10, 2019
@kyroy
Copy link
Owner

kyroy commented Feb 11, 2019

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

@Hiyorimi
Copy link
Author

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.

@kyroy
Copy link
Owner

kyroy commented Apr 19, 2020

@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
Copy link
Author

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

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.

3 participants