-
Notifications
You must be signed in to change notification settings - Fork 13
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 other Minkowski metrics #3
Comments
Hi Kirill, That all sounds right. Pull requests welcome. If you do start working on the distance macros: https://github.com/jefferis/RANN/blob/master/src/ANN/ANN.h#L329-L363 make sure you add the You may also want to take a look at github.com/jefferis/RANN2 (switch to Rcpp, not released to CRAN) and, for template code + Rcpp, https://github.com/jefferis/nabor. |
Thanks, Gregory. Is there a reason why you don't simply upload |
I think the main thing was I hadn't got round to documenting the WANN class (like https://github.com/jefferis/nabor/blob/master/R/WKNNClasses.R). I would also need to go everything with a fine tooth comb to avoid the wrath of BDR on submission and I've been busy! |
I've looked into it, and I've found three options:
Do you have a preference? |
Thanks for looking onto this. I have to say I would be much keener on a one-package solution but having something that works for your case with minimum fuss is important. Do you want to submit this functionality to CRAN? One option would be to keep the existing package for Euclidean distance, but require 2 packages for the other distances - these could be github only initially. Gregory Jefferis On 19 Jan 2015, at 09:36, Kirill Müller notifications@github.com wrote:
|
I'd like to have it on CRAN eventually, but this doesn't have to happen yesterday. Also, I'd need Your idea sounds great -- keeping |
I think the normal mechanism would be to make the other package suggested and to fail gracefully when it is missing. Gregory Jefferis On 19 Jan 2015, at 13:26, Kirill Müller notifications@github.com wrote:
|
I have requested transfer of ownership to you for the brand new RANN1 repository -- I don't want to take credit for your work, but I can also act as maintainer if you prefer. Next would be a pull request that adds a new parameter to |
On the other hand, the RANN1 package should probably live in a branch in this repo. Would you like to create a new branch from RANN1's master? |
I still think a branch in this repo for the other package is the best option. If you create the target branch (perhaps |
Hi Kirill, I’m on sabbatical at the moment, so a bit busy. I just gave you access to the repo. Do you want to make the branch and then do a pull request as suggested. thanks, Greg. On 21 Jan 2015, at 08:26, Kirill Müller notifications@github.com wrote:
Gregory Jefferis, PhD http://www2.mrc-lmb.cam.ac.uk/group-leaders/h-to-m/g-jefferis |
Thanks. I merged the separate unsquare pull. Hope that does not get in the way of the others. I would also have been happy to have the defunct function removal (seems time) in a separate pull but not worth redoing. For the main pulls, I t looks like you're missing a roxygen run, but I'm sure you've already spotted that. |
OK, @krlmlr I think we're all good on this. Thanks for all your work! In detail, merged in 6a935ff (closing #7 and also #11) to remove defunct function, then 59e78b4 (replacing #9) to add your new RANN1 package on a branch of the same name, and finally d7dadc5 (replacing #10) to add info about RANN1 package to main RANN README. |
Looks great to me, thanks for your help! Are you releasing to CRAN? |
Do you mean releasing RANN1 to CRAN or sending the updated RANN (where the changes are essentially cosmetic)? |
I mean |
OK @krlmlr. Would you mind making yourself maintainer of RANN1 and submitting yourself? I have a v2.5 of RANN on master pending with some minor fixes. You will probably want to merge those changes to get a fix for e.g. a title case violation on 3.2 revealed by build_win(). |
I made that suggestion a bit more specific in #13 – hope that's OK! |
The ANN docs suggest that the Euclidean metric is used by default. Some digging in the code and in above docs suggest that this is also true for
RANN
.According to Section 2.2.1 (p. 14 ff.), only the macros in the code linked above need to be redefined to use other Minkowski norms. To define a code base where Manhattan, Euclidean and "max" metric are simultaneously accessible, this probably needs to be replaced by a template parameter that defines the implementation of the four operators.
The purpose is to accelerate statistical matching using the Gower distance: I think I can transform a "Gower distance problem" to a "Manhattan distance problem" but then
RANN
should be able to solve it (it is called by theStatMatch
package).Does this make sense to you?
The text was updated successfully, but these errors were encountered: