Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement ArgMax and ArgMin #20733

Merged
merged 3 commits into from
Nov 28, 2021
Merged

Implement ArgMax and ArgMin #20733

merged 3 commits into from
Nov 28, 2021

Conversation

rogday
Copy link
Member

@rogday rogday commented Sep 22, 2021 •
edited
Loading

Related #20679

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 Apache 2 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

@rogday rogday mentioned this pull request Sep 22, 2021
Closed
alalek
alalek reviewed Sep 24, 2021
View reviewed changes
modules/core/include/opencv2/core.hpp Outdated Show resolved Hide resolved
modules/core/include/opencv2/core.hpp Outdated Show resolved Hide resolved
modules/core/test/test_arithm.cpp Outdated Show resolved Hide resolved
@rogday rogday marked this pull request as draft September 24, 2021 10:25
@rogday rogday marked this pull request as ready for review September 28, 2021 09:09
@rogday rogday requested a review from alalek September 28, 2021 11:23
@asmorkalov
Copy link
Contributor

@alalek could you look at the PR?

@vpisarev
Copy link
Contributor

vpisarev commented Nov 12, 2021 •
edited
Loading

The names of functions are too generic and thus quite confusing. There is minMaxLoc() that finds global minimum and maximum together with their locations. If we don't want to make reduce() heavier, maybe, let's split reduce() into multiple functions and add two more: reduceArgMin() and reduceArgMax().

@rogday rogday marked this pull request as draft November 16, 2021 13:55
@rogday rogday force-pushed the argmaxnd branch 4 times, most recently from 5bdb9ec to 0be4f3f Compare November 16, 2021 20:05
@rogday rogday marked this pull request as ready for review November 16, 2021 21:01
alalek
alalek reviewed Nov 17, 2021
View reviewed changes
modules/core/include/opencv2/core.hpp Outdated Show resolved Hide resolved
modules/core/src/utils/depth_dispatch.hpp Outdated Show resolved Hide resolved
modules/core/src/utils/depth_dispatch.hpp Outdated Show resolved Hide resolved
modules/ts/src/ts_func.cpp Outdated
@@ -2,6 +2,10 @@
#include <float.h>
#include <limits.h>
#include "opencv2/imgproc/types_c.h"
#include "../src/utils/depth_dispatch.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to put this header into core/include/ instead of core/src/

Copy link
Member Author

@rogday rogday Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you help me identify the correct naming/subfolder, so that this header doesn't leak to the public API?
reduce_arg.private.hpp in core/include/opecv2/core/utils or .../private doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

leak to the public API

What do you mean?

  • documentation? - look for //! @cond IGNORED for the whole file
  • install directory? - AFAIK, "private/" handles that.

Implementation itself should be marked as "static inline" (it is template anyway) and may be placed into cv::detail namespace.

  • "core/utils/" - contains low level / system utility functions (not a good place)
  • "core/detail/" - good candidate
  • just "opencv2/core/" should work too (but don't include that into core.hpp or base.hpp)

Preferable name is "dispatch_helper.impl.hpp".

modules/ts/src/ts_func.cpp Outdated Show resolved Hide resolved
@rogday rogday requested a review from alalek November 22, 2021 13:28
alalek
alalek reviewed Nov 22, 2021
View reviewed changes
Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for update!

modules/core/include/opencv2/core/detail/reduce_arg_helper.impl.hpp Outdated Show resolved Hide resolved
modules/core/include/opencv2/core/detail/reduce_arg_helper.impl.hpp Outdated Show resolved Hide resolved
alalek
alalek reviewed Nov 25, 2021
View reviewed changes
Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for update! Looks good to me 👍

modules/core/include/opencv2/core/detail/dispatch_helper.impl.hpp Outdated Show resolved Hide resolved
modules/core/perf/perf_reduce.cpp Outdated
typedef tuple<Size, MatType, int> Size_MatType_RMode_t;
typedef perf::TestBaseWithParam<Size_MatType_RMode_t> Size_MatType_RMode;

PERF_TEST_P(Size_MatType_RMode, reduceArgMinMax, testing::Combine(
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need performance tests now? Can we disable them by default (DISABLED_reduceArgMinMax)?

  • what is about 3D / 4D layouts? (DNN doesn't use 2D matrices in general)

Copy link
Member Author

Choose a reason for hiding this comment

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

3D/4D layouts perform similarly if the number of pixels is the same.

@alalek alalek merged commit f044037 into opencv:4.x Nov 28, 2021
@rogday rogday deleted the argmaxnd branch December 2, 2021 17:15
@zhoujinhai zhoujinhai mentioned this pull request Dec 6, 2021
Closed
This was referenced Dec 7, 2021
Closed
Merged
@alalek alalek mentioned this pull request Dec 30, 2021
Merged
@alalek alalek mentioned this pull request Feb 22, 2022
Merged
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
@rogday
Merge pull request opencv#20733 from rogday:argmaxnd
2892fc9 
Implement ArgMax and ArgMin

* add reduceArgMax and reduceArgMin

* fix review comments

* address review concerns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alalek alalek alalek approved these changes

Assignees

@alalek alalek

Labels
category: core feature test
Projects
None yet
Milestone
4.5.5
Development

Successfully merging this pull request may close these issues.
4 participants
@rogday @asmorkalov @vpisarev @alalek