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 added test feature category: core labels

Sep 22, 2021

rogday mentioned this pull request

Sep 22, 2021

Add test data for minMaxLocNDopencv/opencv_extra#913

Closed

rogday force-pushed theargmaxnd branch fromb53ddcd to7848e96Compare

September 22, 2021 16:19

JulieBar requested a review fromalalek

September 23, 2021 08:33

alalek reviewed

Sep 24, 2021

View reviewed changes

modules/core/include/opencv2/core.hpp OutdatedShow resolvedHide resolved

modules/core/test/test_arithm.cpp OutdatedShow resolvedHide resolved

alalek reviewed

Sep 24, 2021

View reviewed changes

modules/core/include/opencv2/core.hpp OutdatedShow resolvedHide resolved

rogday marked this pull request as draft

September 24, 2021 10:25

rogday marked this pull request as ready for review

September 28, 2021 09:09

rogday requested a review fromalalek

September 28, 2021 11:23

Copy link

Contributor

asmorkalov commentedOct 18, 2021

@alalek could you look at the PR?

Copy link

Contributor

vpisarev commentedNov 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 marked this pull request as draft

November 16, 2021 13:55

rogday force-pushed theargmaxnd branch 4 times, most recently from5bdb9ec to0be4f3fCompare

November 16, 2021 20:05

rogday marked this pull request as ready for review

November 16, 2021 21:01

alalek reviewed

Nov 17, 2021

View reviewed changes

modules/core/include/opencv2/core.hpp OutdatedShow resolvedHide resolved

modules/core/src/utils/depth_dispatch.hpp OutdatedShow resolvedHide resolved

modules/ts/src/ts_func.cpp Outdated

		#include<float.h>
		#include<limits.h>
		#include"opencv2/imgproc/types_c.h"
		#include"../src/utils/depth_dispatch.hpp"

Copy link

Member

alalekNov 17, 2021

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 intocore/include/ instead ofcore/src/

Copy link

MemberAuthor

rogdayNov 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 incore/include/opecv2/core/utils or.../private doesn't work.

Copy link

Member

alalekNov 18, 2021

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 intocv::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 OutdatedShow resolvedHide resolved

rogday force-pushed theargmaxnd branch from0be4f3f toc45696dCompare

November 18, 2021 16:56

add reduceArgMax and reduceArgMin

b81b42b

rogday force-pushed theargmaxnd branch fromc45696d tob81b42bCompare

November 18, 2021 17:50

rogday requested a review fromalalek

November 22, 2021 13:28

alalek reviewed

Nov 22, 2021

View reviewed changes

Copy link

Member

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 OutdatedShow resolvedHide resolved

modules/core/include/opencv2/core/detail/reduce_arg_helper.impl.hppShow resolvedHide resolved

fix review comments

eb91ba8

alalek reviewed

Nov 25, 2021

View reviewed changes

Copy link

Member

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 OutdatedShow resolvedHide 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

alalekNov 25, 2021

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

MemberAuthor

rogdayNov 26, 2021

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.

address review concerns

b740ea4

alalek approved these changes

Nov 28, 2021

View reviewed changes

alalek merged commitf044037 intoopencv:4.x

Nov 28, 2021

rogday deleted the argmaxnd branch

December 2, 2021 17:15

zhoujinhai mentioned this pull request

Dec 6, 2021

"Argmax" operate is not supported by dnn?#21200

Closed

This was referencedDec 7, 2021

Add support for new type of reduce(): REDUCE_MAX_IND#10286

Closed

Add ArgMax and ArgMin layers#21208

Merged

alalek mentioned this pull request

Dec 30, 2021

(5.x) Merge 4.x#21371

Merged

alalek mentioned this pull request

Feb 22, 2022

(5.x) Merge 4.x#21651

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request

Mar 30, 2023

Merge pull requestopencv#20733from rogday:argmaxnd

2892fc9

Implement ArgMax and ArgMin* add reduceArgMax and reduceArgMin* fix review comments* address review concerns

Labels

category: core feature test

Movatterモバイル変換

Uh oh!

Implement ArgMax and ArgMin#20733

Implement ArgMax and ArgMin#20733

Conversation

rogday commentedSep 22, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asmorkalov commentedOct 18, 2021

Uh oh!

vpisarev commentedNov 12, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alalekNov 17, 2021

Choose a reason for hiding this comment

Uh oh!

rogdayNov 18, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalekNov 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalekNov 25, 2021

Choose a reason for hiding this comment

Uh oh!

rogdayNov 26, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rogday commentedSep 22, 2021•
edited
Loading

vpisarev commentedNov 12, 2021•
edited
Loading

rogdayNov 18, 2021•
edited
Loading