Skip to content

Update kdtree kr with dist (and refactor)#303

Merged
tremblap merged 14 commits into
flucoma:mainfrom
tremblap:update/kdtree-kr-with-dist-and-refactor
Jun 13, 2025
Merged

Update kdtree kr with dist (and refactor)#303
tremblap merged 14 commits into
flucoma:mainfrom
tremblap:update/kdtree-kr-with-dist-and-refactor

Conversation

@tremblap
Copy link
Copy Markdown
Member

Using the latest refactor in DS.kr, enacted the following changes:

  1. made the variable names more explicit (mLastNumPoints and kLookupDataSet)
  2. removed the temp output vector and its allocator
  3. replaced loops with sdt::iterators
  4. added the distance output when no lookupDataSet is provided

still 2 questions pending:

  1. line 230: I wasn't able to remove the 'point' allocation, as the first argument of kdtree.algo.knearest is not templatable for me
  2. line 243: I needed to dereference id - i don't know why, as I though that id would be an iteration of ids, each being a pointer to a string in the vector of strings pointers, like in DataSet.kr

any pointer (pun intended) welcome

@tremblap tremblap requested a review from weefuzzy April 16, 2025 06:56
@tremblap
Copy link
Copy Markdown
Member Author

the 2nd commit also corrects the error for non-triggered kr output post negative triggered output

@weefuzzy
Copy link
Copy Markdown
Member

  • line 230: I wasn't able to remove the 'point' allocation, as the first argument of kdtree.algo.knearest is not templatable for me

It's not calling into already generic code, so there would be some extra work in updating other functions too. Given the amount of other apparatus querying KDTree creates, the pay off from avoiding this one temporary pales a bit.

  • line 243: I needed to dereference id - i don't know why, as I though that id would be an iteration of ids, each being a pointer to a string in the vector of strings pointers, like in DataSet.kr

There's no particular reason to suppose it should work like the query for DataSet. kNearest here returns a KNNResult which is a pair<vector<double>, vector<const string*>>. So you need to deference the id when you call get because it's a string* and you need a string& there.

Comment thread include/flucoma/clients/nrt/KDTreeClient.hpp Outdated
@tremblap
Copy link
Copy Markdown
Member Author

thanks for this, pushed the change. Now, I have avoided the TODO at line 108, mostly because I didn't know how to overload a single function with a fork in the end... or maybe I have a private (computeKnearest that is called with a bool argument on output one or the other?)

@tremblap tremblap closed this Jun 11, 2025
@tremblap tremblap deleted the update/kdtree-kr-with-dist-and-refactor branch June 11, 2025 14:37
@tremblap tremblap restored the update/kdtree-kr-with-dist-and-refactor branch June 11, 2025 14:39
@tremblap tremblap reopened this Jun 11, 2025
@tremblap
Copy link
Copy Markdown
Member Author

I have 2 c++ potential improvments to be checked by @weefuzzy here.

  1. line 250-251 - no need to square there, so I reckon I could just make a slice but I couldn't find the right syntax
  2. line 84-85 seem to be the only way I managed to compile - otherwise I got the ugly error of <Cannot decompose class type 'MessageResult<std::pair<std::vector<double,...'

happy to learn again - or to merge as is because life is too short

BufferAdaptor::ReadAccess(data.get()).samps(0, mAlgorithm.dims(), 0);
auto [dist, ids] = mAlgorithm.kNearest(point, k, get<kRadius>());
return {dist};
auto reply = computeKnearest(data, k);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to check that the result is ok here as well

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oupsy, sorry, added now running the tests

@tremblap tremblap requested a review from weefuzzy June 13, 2025 07:42
…and 0 to get all ranked, so we just need to check neither are negative.
@tremblap tremblap merged commit d02740c into flucoma:main Jun 13, 2025
3 checks passed
@tremblap tremblap deleted the update/kdtree-kr-with-dist-and-refactor branch June 13, 2025 08:53
weefuzzy pushed a commit to weefuzzy/flucoma-core that referenced this pull request Nov 25, 2025
* first working KDTreeQuery refactor with questions

* corrected the error for non-triggered state

* wise catch from a wise man

* even better when it compiles

* formated

* 3rd time lucky sorry for the noise

* now with a shared function

* fix error (no squaring needed here as the algo layer does that for us)

* much better / much terser copy

* adds the replies

* forgot the test there...

* this condition used to be commented (with <=) but in effect we use 0 and 0 to get all ranked, so we just need to check neither are negative.
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