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

core: add broadcast #23965

Merged
merged 12 commits into from Aug 30, 2023
Merged

core: add broadcast #23965

merged 12 commits into from Aug 30, 2023

Conversation

fengyuentau
Copy link
Member

@fengyuentau fengyuentau commented Jul 11, 2023 •
edited by opencv-alalek

Should work as np.broadcast_to.

Benchmarks

Results on M1:

src dst v1 (mean, ms) v2 (mean, ms)
[1, 10, 100, 1000] [1000, 10, 100, 1000] 124.79 63.41
[1000, 1, 100, 1000] [1000, 10, 100, 1000] 866.13 89.96
[1000, 10, 1, 1000] [1000, 10, 100, 1000] 231.30 93.71
[1000, 10, 100, 1] [1000, 10, 100, 1000] 3798.67 100.02

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 another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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
force_builders=Win32,Linux32,Linux AVX2

asmorkalov
asmorkalov reviewed Jul 11, 2023
View reviewed changes
modules/core/include/opencv2/core.hpp Outdated
* @param dst output array that has the given shape
* @param shape target shape
*/
CV_EXPORTS_W void broadcast_to(InputArray src, const std::vector<int>& shape, OutputArray dst);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use OpenCV naming conventions. Should be broadcastTo

Copy link
Contributor

Choose a reason for hiding this comment

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

@vpisarev Should it be Mat class method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please use OpenCV naming conventions. Should be broadcastTo

Done.

@asmorkalov asmorkalov requested a review from vpisarev July 11, 2023 08:45
@asmorkalov asmorkalov added this to the 4.9.0 milestone Jul 11, 2023
@fengyuentau fengyuentau changed the title core: add broadcast_to core: add broadcastTo Jul 11, 2023
@fengyuentau fengyuentau changed the title core: add broadcastTo core: add broadcast Aug 3, 2023
opencv-alalek
opencv-alalek reviewed Aug 18, 2023
View reviewed changes
modules/core/include/opencv2/core.hpp Outdated Show resolved Hide resolved
modules/core/src/matrix_transform.cpp Outdated Show resolved Hide resolved
modules/core/src/matrix_transform.cpp Outdated Show resolved Hide resolved
modules/core/src/matrix_transform.cpp Show resolved Hide resolved
modules/core/test/test_arithm.cpp Outdated Show resolved Hide resolved
modules/core/test/test_arithm.cpp Outdated Show resolved Hide resolved
modules/core/perf/perf_arithm.cpp Outdated Show resolved Hide resolved
@opencv-alalek
Copy link
Contributor

opencv-alalek commented Aug 18, 2023 •
edited

Please take a look on failures of 32-bit builds: http://pullrequest.opencv.org/buildbot/builders/precommit_linux32/builds/100233

Failed to allocate 4000000000 bytes

4GiB is too much for any OpenCV test.
Keep it less than 100Mb.

@fengyuentau
Copy link
Member Author

Please take a look on failures of 32-bit builds: http://pullrequest.opencv.org/buildbot/builders/precommit_linux32/builds/100233

Failed to allocate 4000000000 bytes

4GiB is too much for any OpenCV test. Keep it less than 100Mb.

Target scale is down to {10, 100, 1000} now, which is about 3.8MB.

@fengyuentau
Copy link
Member Author

@opencv-alalek friendly reminder.

@fengyuentau
Copy link
Member Author

Hello, any reviewer updates for this pull request?

@vpisarev vpisarev merged commit a308dfc into opencv:4.x Aug 30, 2023
23 checks passed
@fengyuentau fengyuentau deleted the broadcast_to branch August 30, 2023 06:54
@dkurt dkurt mentioned this pull request Sep 11, 2023
Merged
13 tasks
@asmorkalov asmorkalov mentioned this pull request Sep 11, 2023
Merged
@hipudding hipudding mentioned this pull request Sep 22, 2023
Closed
5 tasks
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
@fengyuentau @thewoz
core: add broadcast (opencv#23965)
a320dd5 
* add broadcast_to with tests

* change name

* fix test

* fix implicit type conversion

* replace type of shape with InputArray

* add perf test

* add perf tests which takes care of axis

* v2 from ficus expand

* rename to broadcast

* use randu in place of declare

* doc improvement; smaller scale in perf

* capture get_index by reference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opencv-alalek opencv-alalek opencv-alalek left review comments

@vpisarev vpisarev vpisarev approved these changes

@asmorkalov asmorkalov Awaiting requested review from asmorkalov

Assignees

@vpisarev vpisarev

Labels
category: core feature
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fengyuentau @opencv-alalek @vpisarev @asmorkalov