Skip to content

Fix: error in the dimension used for computeMinMax#17607

Closed
pemmanuelviel wants to merge 1 commit intoopencv:masterfrom
pemmanuelviel:pev-fix-middleSplit-for-kdtree-single
Closed

Fix: error in the dimension used for computeMinMax#17607
pemmanuelviel wants to merge 1 commit intoopencv:masterfrom
pemmanuelviel:pev-fix-middleSplit-for-kdtree-single

Conversation

@pemmanuelviel
Copy link
Copy Markdown
Contributor

@pemmanuelviel pemmanuelviel commented Jun 21, 2020 •
edited
Loading

Instead of using the current dimension for which we just got a big span,
we were computing Min and Max for the previous dimension stored in cutfeat
(and using 0 instead of the dimension indice for the very first dimension
with "span > (1-eps)max_span")

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under OpenCV (BSD) License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@pemmanuelviel
Fix: error in the dimension used for computeMinMax
9e7225b 
Instead of using the current dimension for which we just got a big span,
we were computing Min and Max for the previous dimension stored in cutfeat
(and using 0 instead of the dimension indice for the very first dimension
with "span > (1-eps)max_span")
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jun 22, 2020

@pemmanuelviel Thanks for the patch. Could you provide small test that proofs your fix?

@asmorkalov asmorkalov added bug pr: needs test New functionality requires minimal tests set labels Jun 22, 2020
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jun 22, 2020

@pemmanuelviel Also it looks like the same issue is present in 3.4 branch too. This patch should go into 3.4 branch first. We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

So, please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no need to re-open PR, apply changes "inplace".

@asmorkalov asmorkalov added the pr: needs rebase Rebase patch (and squash fixup commits) on the top of target branch label Jun 22, 2020
@vpisarev vpisarev self-requested a review June 22, 2020 11:54
@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented Jun 22, 2020

@pemmanuelviel, thank you, looks good to me! 👍
I think, since it's the change in a single file, maybe it would be enough just to change the target branch from master to 3.4 right in your pull request without any git commands

@pemmanuelviel
Copy link
Copy Markdown
Contributor Author

pemmanuelviel commented Jun 22, 2020

Hi @vpisarev ,
Just changing the target branch is not enough as merging will not work.
I will backport this change and some others in 3.4 following @asmorkalov advices after I've finished fixing the issues spotted by the buildfarm on my different submitted branches.

@pemmanuelviel
Copy link
Copy Markdown
Contributor Author

pemmanuelviel commented Jun 23, 2020

@asmorkalov To ensure each commit is fine on 3.4 I've created new branches (replacing pev-xxx by pev--xxx) and cherry-picked each commit. These new branches are currently in pull-request.

@pemmanuelviel
Copy link
Copy Markdown
Contributor Author

pemmanuelviel commented Jun 23, 2020

BTW, how can I associate labels to my PR? Can't find where in the gui...

@pemmanuelviel pemmanuelviel mentioned this pull request Jun 25, 2020
Merged
6 tasks
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jun 26, 2020

@pemmanuelviel Only maintainers add labels to PRs.

@alalek alalek mentioned this pull request Jun 29, 2020
Merged
@pemmanuelviel pemmanuelviel deleted the pev-fix-middleSplit-for-kdtree-single branch July 2, 2020 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vpisarev vpisarev vpisarev approved these changes

Assignees

No one assigned

Labels

bug pr: needs rebase Rebase patch (and squash fixup commits) on the top of target branch pr: needs test New functionality requires minimal tests set

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pemmanuelviel @asmorkalov @vpisarev