Skip to content

LSTM ONNX Layout Attribute Support #23614

Merged
asmorkalov merged 7 commits intoopencv:4.xfrom
Abdurrahheem:lstm_layout_attribute
May 17, 2023
Merged

LSTM ONNX Layout Attribute Support #23614
asmorkalov merged 7 commits intoopencv:4.xfrom
Abdurrahheem:lstm_layout_attribute

Conversation

@Abdurrahheem
Copy link
Copy Markdown
Contributor

@Abdurrahheem Abdurrahheem commented May 15, 2023 •
edited by dkurt
Loading

Explanation

This PR contains necessary changes to support layout attribute. This attributes is present in ONNX and Torch (in touch it is name as batch_first=True) libraries. When layout = 1 input to LSTM layer is expected to have batch dimension first -> [batch_size, sequence_length, features] vs layout = 0 - default [sequence_length, batch_size, features]

resolves #23602

Test Data

Test data and data generator for PR located here #1063

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

@Abdurrahheem Abdurrahheem mentioned this pull request May 15, 2023
Merged
@Abdurrahheem Abdurrahheem changed the title Lstm layout attribute LSTM Layout Attribute Support May 15, 2023
@Abdurrahheem Abdurrahheem self-assigned this May 15, 2023
@Abdurrahheem Abdurrahheem added the category: dnn (onnx) ONNX suport issues in DNN module label May 15, 2023
@Abdurrahheem Abdurrahheem force-pushed the lstm_layout_attribute branch from 2d9641a to 7025e65 Compare May 15, 2023 11:50
@asmorkalov asmorkalov changed the title LSTM Layout Attribute Support LSTM ONNX Layout Attribute Support May 15, 2023
@asmorkalov asmorkalov removed the request for review from rogday May 15, 2023 12:38
@asmorkalov asmorkalov added this to the 4.8.0 milestone May 15, 2023
@Abdurrahheem Abdurrahheem marked this pull request as ready for review May 15, 2023 13:05
@Abdurrahheem Abdurrahheem requested a review from rogday May 15, 2023 13:05
@asmorkalov asmorkalov requested review from fengyuentau and removed request for asmorkalov and rogday May 16, 2023 07:11
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 16, 2023

@fengyuentau @dkurt Could you look at it?

fengyuentau
fengyuentau approved these changes May 17, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau left a comment

Choose a reason for hiding this comment

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

👍

dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt reviewed May 17, 2023
View reviewed changes
modules/dnn/src/onnx/onnx_importer.cpp Outdated
bool need_yc = lstm_proto.output_size() > 2 && !lstm_proto.output(2).empty();
bool need_yh = lstm_proto.output_size() > 1 && !lstm_proto.output(1).empty();
bool need_y = lstm_proto.output_size() > 0 && !lstm_proto.output(0).empty();
bool need_y = lstm_proto.output_size() > 0 && !lstm_proto.output(0).empty();
Copy link
Copy Markdown
Member

@dkurt dkurt May 17, 2023

Choose a reason for hiding this comment

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

please revert

Copy link
Copy Markdown
Contributor Author

@Abdurrahheem Abdurrahheem May 17, 2023

Choose a reason for hiding this comment

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

Done! But why?

Copy link
Copy Markdown
Member

@dkurt dkurt May 17, 2023

Choose a reason for hiding this comment

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

Just simpler for the future tracking of changed lines.

dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt requested changes May 17, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dkurt dkurt left a comment

Choose a reason for hiding this comment

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

Please see comments. The main one is to introduce internal enum for different LSTM layouts (sequence, batch) instead of a boolean flag.

dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt reviewed May 17, 2023
View reviewed changes
dkurt
dkurt reviewed May 17, 2023
View reviewed changes
@asmorkalov asmorkalov merged commit d2143bc into opencv:4.x May 17, 2023
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
Merged
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
@Abdurrahheem @thewoz
Merge pull request opencv#23614 from Abdurrahheem:lstm_layout_attribute
f4085f9 
LSTM ONNX Layout Attribute Support opencv#23614 

### Explanation

This PR contains necessary changes to support `layout` attribute. This attributes is present in [ONNX](https://github.com/onnx/onnx/blob/main/docs/Operators.md#lstm) and [Torch](https://pytorch.org/docs/stable/generated/torch.nn.LSTM.html#lstm) (in touch it is name as `batch_first=True`) libraries. When `layout = 1` input to LSTM layer is expected to have batch dimension first -> `[batch_size, sequence_length, features]` vs `layout = 0` - default `[sequence_length, batch_size, features]`

### Test Data

Test data and data generator for PR located here [opencv#1063](opencv/opencv_extra#1063)

### 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
- [ ] There is a reference to the original bug report and related work
- [x] 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 May 29, 2024
@Abdurrahheem @thewoz
Merge pull request opencv#23614 from Abdurrahheem:lstm_layout_attribute
ab7c411 
LSTM ONNX Layout Attribute Support opencv#23614 

### Explanation

This PR contains necessary changes to support `layout` attribute. This attributes is present in [ONNX](https://github.com/onnx/onnx/blob/main/docs/Operators.md#lstm) and [Torch](https://pytorch.org/docs/stable/generated/torch.nn.LSTM.html#lstm) (in touch it is name as `batch_first=True`) libraries. When `layout = 1` input to LSTM layer is expected to have batch dimension first -> `[batch_size, sequence_length, features]` vs `layout = 0` - default `[sequence_length, batch_size, features]`

### Test Data

Test data and data generator for PR located here [opencv#1063](opencv/opencv_extra#1063)

### 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
- [ ] There is a reference to the original bug report and related work
- [x] 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

Reviewers

@fengyuentau fengyuentau fengyuentau approved these changes

@dkurt dkurt dkurt approved these changes

Assignees

@Abdurrahheem Abdurrahheem

Labels

category: dnn (onnx) ONNX suport issues in DNN module feature

Projects

None yet

Milestone

4.8.0

Development

Successfully merging this pull request may close these issues.

LSTM layer with batch_firs=True fails

4 participants

@Abdurrahheem @asmorkalov @fengyuentau @dkurt