Skip to content

Conversation

BTV25
Copy link

@BTV25 BTV25 commented May 10, 2025

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 BTV25 mentioned this pull request Jul 9, 2025
@KristofferC
Copy link
Owner

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

@BTV25
Copy link
Author

BTV25 commented Jul 11, 2025

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?

@KristofferC
Copy link
Owner

KristofferC commented Jul 11, 2025

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.

@BTV25
Copy link
Author

BTV25 commented Jul 11, 2025

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

@KristofferC
Copy link
Owner

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

@BTV25
Copy link
Author

BTV25 commented Jul 11, 2025

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

@KristofferC
Copy link
Owner

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.

@BTV25
Copy link
Author

BTV25 commented Jul 12, 2025

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

BTV25 commented Jul 15, 2025

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

@KristofferC
Copy link
Owner

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.

@BTV25
Copy link
Author

BTV25 commented Jul 18, 2025

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

@KristofferC
Copy link
Owner

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.

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.

2 participants