Conversation
|
@alalek could you look at the PR? |
|
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(). |
5bdb9ec to
0be4f3f
Compare
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" |
There was a problem hiding this comment.
It makes sense to put this header into core/include/ instead of core/src/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
leak to the public API
What do you mean?
- documentation? - look for
//! @cond IGNOREDfor 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/core/include/opencv2/core/detail/reduce_arg_helper.impl.hpp
Outdated
Show resolved
Hide resolved
alalek
left a comment
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
3D/4D layouts perform similarly if the number of pixels is the same.
Implement ArgMax and ArgMin * add reduceArgMax and reduceArgMin * fix review comments * address review concerns
Related #20679
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.