Skip to content

FLANN::knnSearch kNN assertion#10549

Merged
opencv-pushbot merged 1 commit intomasterfrom
unknown repository
Jan 10, 2018
Merged

FLANN::knnSearch kNN assertion#10549
opencv-pushbot merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 8, 2018

resolves #10548

This pullrequest changes

  • Prevent the user from using kNN values larger than the dataset size

int dtype = DataType<DistanceType>::type;
IndexType* index_ = (IndexType*)index;

CV_Assert((size_t)knn <= index_->size());
Copy link
Copy Markdown
Author

@ghost ghost Jan 8, 2018

Choose a reason for hiding this comment

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

There are two main strategies:

  • Failure-avoidance
    • Set knn = MIN(knn, index_->size());
    • Return NaNs for the additional kNNs.
  • Raise an exception to reject the user input

Is it my imagination, or this is also related to #8300? In that case, I guess the vote for this matter is meaningless, as there are cases where one strategy is better than the other, and vise versa.

In this specific case, the user is expecting a matrix of kNN width, returning smaller one is not a sane choice. Moreover, while returning NaNs in the distance makes sense, the indices matrix needs to be int-type. Accordingly, an exception is the best choice, despite being a fan of failure-avoidance, which does not make sense here.

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.

#8300 is about an empty cv::Mat as input parameter. Is empty cv::Mat an invalid parameter or not?

This parameters check is fine here.

@ghost ghost changed the title FIX - FLANN::knnSearch garbage bug FLANN::knnSearch kNN assertion Jan 8, 2018
@opencv-pushbot opencv-pushbot merged commit 8943441 into opencv:master Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FLANN::knnSearch does not assert the kNN value

3 participants