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

support new metric argument for nn2() #6

Closed
wants to merge 8 commits into from
Closed

support new metric argument for nn2() #6

wants to merge 8 commits into from

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Jan 22, 2015

  • calls are forwarded to RANN1
  • documentation in the main README file

Fixes #3.

Closes #4 (=contains it).

@krlmlr
Copy link
Collaborator Author

krlmlr commented Jan 22, 2015

Now, get_nn2_func uses an environment for lookup, the median run time jumps around 10 microseconds per call.

> microbenchmark(RANN:::get_nn2_func("manhattan"), times = 100000)
Unit: microseconds
                             expr   min    lq     mean median     uq      max neval
 RANN:::get_nn2_func("manhattan") 4.771 6.067 10.57319  7.057 14.842 1706.912 1e+05

@jefferis
Copy link
Member

The use of the environment is a nice way to speed things up. My only comment now would be: should we just bypass worker_nn2 for Euclidean distance? That way the implementation is essentially unchanged for the standard Euclidean distance.

Either that or prefill the environment in onLoad

@krlmlr
Copy link
Collaborator Author

krlmlr commented Jan 26, 2015

For a long-term solution, I think it's best if the master and master-L1 branches are as similar as possible. I'll find a solution with minimal penalty for the "default" case.

@jefferis
Copy link
Member

So the conclusion for the time being is that we simply refer to your new RANN1 package in the docs as implemented in your #10. This PR has therefore been superseded so I am closing it (even though some of the approaches you tried here were interesting).

@jefferis jefferis closed this Apr 14, 2015
@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 14, 2015

Fine with me, thanks.

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.

Support other Minkowski metrics
2 participants