Ellipses supported added for Einsum Layer #24322

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

Merged

asmorkalov merged 6 commits into opencv:4.x from Abdurrahheem:ash/dev_einsum_ellips

Oct 24, 2023

Contributor

Abdurrahheem commented Sep 26, 2023 •

edited

Loading

This PR added addresses issues not covered in #24037. Namely these are:
Test case for this patch is in this PR #1106 in opencv extra

Added:

Broadcasting reduction "...ii ->...I"
Add lazy shape deduction. "...ij, ...jk->...ik"

Features to add:

Add implicit output computation support. "bij,bjk ->" (output subscripts should be "bik")
Add support for CUDA backend
BatchWiseMultiply optimize
Performance test

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


          squashed commits for einsum

d9ad679

Abdurrahheem added the category: dnn (onnx) label

Abdurrahheem added this to the 4.9.0 milestone

Abdurrahheem requested review from asmorkalov, dkurt, fengyuentau and opencv-alalek

September 26, 2023 06:56

Abdurrahheem self-assigned this

Contributor

asmorkalov commented Sep 27, 2023

Run cd $HOME/build && python3 $HOME/scripts/warnings-handling.py
/home/ci/opencv/modules/dnn/src/layers/einsum_layer.cpp:1057:68: warning: value computed is not used [-Wunused-value]


          deleted unsed value

e1ec160

asmorkalov marked this pull request as ready for review

October 2, 2023 08:46

asmorkalov added the feature label

Contributor

asmorkalov commented Oct 2, 2023

@Abdurrahheem Thanks a lot for the patch! The PR adds 2 features, but only one test. Could you cover both?

asmorkalov reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp Outdated Show resolved Hide resolved

modules/dnn/src/layers/einsum_layer.cpp Outdated Show resolved Hide resolved

modules/dnn/src/layers/einsum_layer.cpp Show resolved Hide resolved


          fix MR comments

aeca7be

Abdurrahheem mentioned this pull request

Test case for Einsum ellipse functionality opencv/opencv_extra#1106

Merged

asmorkalov reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp Show resolved Hide resolved

modules/dnn/src/layers/einsum_layer.cpp Outdated Show resolved Hide resolved

modules/dnn/src/layers/einsum_layer.cpp Outdated

    
                  for (int slice = 0; slice < total_slices; ++slice) {

                      for (int j = 0; j < inner_stride; ++j) {

                          // Extract diagonal elements

                          output.at<T>(slice, j, 0) = input.at<T>(slice, j, j);

Contributor

asmorkalov Oct 3, 2023

Looks like the 3rd dimension is redundant. You can create output mat with 2 dimensions and drop reshape at the end.

Contributor Author

Abdurrahheem Oct 5, 2023

tested suggestion. This breaks the the tests. Let's leave as it is maybe? since the suggestion only reduces one reshape operation


          PR comments fix

38226f2

dkurt reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp Outdated Show resolved Hide resolved

fengyuentau reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp Show resolved Hide resolved

modules/dnn/src/layers/einsum_layer.cpp Outdated Show resolved Hide resolved


          fix comments

f503ceb

Contributor

asmorkalov commented Oct 11, 2023

@dkurt @fengyuentau Please take a look again.

dkurt reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp Outdated

    
                  }

                  // Reshape output back to original shape

                  original_dims[rank - 1] = 1;  // Set the last dimension to 1, as we have extracted the diagonal

Member

dkurt Oct 11, 2023

Why not initialize output with the final dimensions once but int the loop work with a data pointer?

Contributor Author

Abdurrahheem Oct 23, 2023

changed. now without reshape

dkurt reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp Outdated

    
                  for (int slice = 0; slice < total_slices; ++slice) {

                      for (int j = 0; j < inner_stride; ++j) {

                          // Extract diagonal elements

                          output.at<T>(slice, j, 0) = input.at<T>(slice, j, j);

Member

dkurt Oct 11, 2023

This method can be without template by using cv::Mat::diag:

input = input(1, total_slices);
output = output.reshape(1, total_slices);
for (int slice = 0; slice < total_slices; ++slice) {
  Mat src = input.row(slice).reshape(1, inner_stride).diag(0);
  Mat dst = output.row(slice);
  src.copyTo(dst);
}

Contributor Author

Abdurrahheem Oct 23, 2023

tested, this introduces regression in accuracy

dkurt reviewed

View reviewed changes

modules/dnn/src/layers/einsum_layer.cpp

    
                  if (output_dims != shape(output)){

                      CV_Error(Error::StsError, "Output shape does not match with calculated shape");

                  }

Member

dkurt Oct 11, 2023

Do we really need output_dims as it's used only for the check?

Contributor Author

Abdurrahheem Oct 23, 2023

do we not need to check?

Member

dkurt Oct 24, 2023

We probably initialize output somewhere once in the code with such shapes.

Contributor Author

Abdurrahheem Oct 24, 2023

output shape for this particular function is not saved anywhere. However, overall output shape is save, of course. Because of this I need to check the outputs for compatibility.


          fix MR comments

101aae3

Contributor

asmorkalov commented Oct 24, 2023

@dkurt please take a look again.

dkurt approved these changes

View reviewed changes

asmorkalov assigned dkurt and unassigned Abdurrahheem

asmorkalov merged commit a3b3a58 into opencv:4.x

asmorkalov pushed a commit to opencv/opencv_extra that referenced this pull request


          Merge pull request #1106 from Abdurrahheem:ash/dev_einsum_ellips

c324ea9

Test case for Einsum ellipse functionality #1106

This PR contains test case for Einsum Ellipses addition patch [#24322](opencv/opencv#24322)

asmorkalov mentioned this pull request

(5.x) Merge 4.x #24486

Merged

IskXCr pushed a commit to Haosonn/opencv that referenced this pull request


          Merge pull request opencv#24322 from Abdurrahheem:ash/dev_einsum_ellips

4cac695

Ellipses supported added for Einsum Layer opencv#24322

This PR added addresses issues not covered in opencv#24037. Namely these are: 
Test case for this patch is in this PR [opencv#1106](opencv/opencv_extra#1106) in opencv extra

Added: 
 - [x] Broadcasting reduction "...ii ->...I"
 - [x] Add lazy shape deduction. "...ij, ...jk->...ik"
 
 Features to add: 
- [ ] Add implicit output computation support. "bij,bjk ->" (output subscripts should be "bik")
- [ ] Add support for CUDA backend 
- [ ] BatchWiseMultiply optimize
- [ ] Performance test

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] 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.
- [x] The feature is well documented and sample code can be built with the project CMake

opencv-alalek removed their request for review

December 26, 2023 20:45

thewoz pushed a commit to thewoz/opencv that referenced this pull request


          Merge pull request opencv#24322 from Abdurrahheem:ash/dev_einsum_ellips

5c7874e

Ellipses supported added for Einsum Layer opencv#24322

This PR added addresses issues not covered in opencv#24037. Namely these are: 
Test case for this patch is in this PR [opencv#1106](opencv/opencv_extra#1106) in opencv extra

Added: 
 - [x] Broadcasting reduction "...ii ->...I"
 - [x] Add lazy shape deduction. "...ij, ...jk->...ik"
 
 Features to add: 
- [ ] Add implicit output computation support. "bij,bjk ->" (output subscripts should be "bik")
- [ ] Add support for CUDA backend 
- [ ] BatchWiseMultiply optimize
- [ ] Performance test

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] 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.
- [x] The feature is well documented and sample code can be built with the project CMake

thewoz pushed a commit to thewoz/opencv that referenced this pull request


          Merge pull request opencv#24322 from Abdurrahheem:ash/dev_einsum_ellips

efeabe7

Ellipses supported added for Einsum Layer opencv#24322

This PR added addresses issues not covered in opencv#24037. Namely these are: 
Test case for this patch is in this PR [opencv#1106](opencv/opencv_extra#1106) in opencv extra

Added: 
 - [x] Broadcasting reduction "...ii ->...I"
 - [x] Add lazy shape deduction. "...ij, ...jk->...ik"
 
 Features to add: 
- [ ] Add implicit output computation support. "bij,bjk ->" (output subscripts should be "bik")
- [ ] Add support for CUDA backend 
- [ ] BatchWiseMultiply optimize
- [ ] Performance test

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] 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.
- [x] The feature is well documented and sample code can be built with the project CMake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: dnn (onnx) feature