NearestNeighbors.jl icon indicating copy to clipboard operation
NearestNeighbors.jl copied to clipboard

Runtime function

Open BTV25 opened this issue 10 months ago • 12 comments

I have added a function: inrange_runtime!(tree::NNTree{V}, points::AbstractVector{T}, radius::Number, runtime_function::Function) that takes in a function as the last argument, and rather than returning the in-range indices, sends the indices to the runtime function to perform some operation.

I added a test and docstring

BTV25 avatar May 10 '25 20:05 BTV25

Can the current functionality for inrange be implemented as a special choice of a runtime function?

KristofferC avatar Jul 11 '25 13:07 KristofferC

I believe so. Are you thinking it would be better to add the runtime_function argument directly to the inrange function as a keyword and use a default function to imitate the current functionality?

BTV25 avatar Jul 11 '25 14:07 BTV25

Something like:

inrange(...) = inrange_runtime(..., runtime=index_returning_runtime_function)

is what I thought. So that inrange is just a inrange_runtime call with a suitable chosen runtime function. Something like callback might be more descriptive than runtime_function also, I feel.

KristofferC avatar Jul 11 '25 14:07 KristofferC

Okay @KristofferC I think I have completed the updates. What do you think?

BTV25 avatar Jul 11 '25 20:07 BTV25

By the way, inrangecount is also a special case of this functionality, right?

KristofferC avatar Jul 11 '25 21:07 KristofferC

Yes it is, but unfortunately it does not perform as well as the current implementation. It does save some memory but takes longer.

BTV25 avatar Jul 11 '25 22:07 BTV25

Any idea why that would be? There should be no difference in the ideal case, or? Would perhaps be good to try figure out so that we don't leave performance on the table for the runtime function functionality.

KristofferC avatar Jul 12 '25 18:07 KristofferC

If I use a callback function I have to give it some sort of storage that is an array so I can modify the count in place. At that point get_index for that count takes extra time that the current implimentation does not have to.

BTV25 avatar Jul 12 '25 21:07 BTV25

If there isn't anything else, the code should be ready to merge.

BTV25 avatar Jul 15 '25 14:07 BTV25

I think this needs a rebase against master or something like that because as it is now it seems to have many unrelated changes like removing some recent updates to dosctrings etc.

Also, I want to go through this properly myself before merging. Right now, I am busy with preparing for JuliaCon so it may take a little while.

KristofferC avatar Jul 18 '25 11:07 KristofferC

That is understandable. If you need anything from me just let me know.

BTV25 avatar Jul 18 '25 15:07 BTV25

I just want to say this is not forgotten but that I am on vacation after JuliaCon and I have limited access to a computer and time etc. Should be back to normal next week.

KristofferC avatar Aug 06 '25 18:08 KristofferC

Just pinging this to make sure it doesn't get lost @KristofferC

BTV25 avatar Jan 17 '26 20:01 BTV25