Skip to content

Add Python's test for LSTM layer#20480

Merged
alalek merged 3 commits intoopencv:3.4from
JulieBar:lstm_pytest
Aug 5, 2021
Merged

Add Python's test for LSTM layer#20480
alalek merged 3 commits intoopencv:3.4from
JulieBar:lstm_pytest

Conversation

@JulieBar
Copy link
Copy Markdown
Contributor

@JulieBar JulieBar commented Jul 30, 2021

Add test for workaround mentioned at issue #18880

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

@JulieBar JulieBar requested a review from asmorkalov July 30, 2021 16:12
asmorkalov
asmorkalov approved these changes Aug 2, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 2, 2021

@JulieBar There is some accuracy issue on CI builds with OpenCL. Please take a look. Most probably we have to tune epsilon here.

@asmorkalov asmorkalov self-requested a review August 2, 2021 09:02
alalek
alalek reviewed Aug 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Accuracy tests for C++ code should be done as C++ tests.
Python tests should validate bindings to C++ code.

We should not duplicate tests if this doesn't increase code coverage.

@JulieBar
Copy link
Copy Markdown
Contributor Author

JulieBar commented Aug 2, 2021

@alalek It's not an accuracy test. This test checks that LSTM layer works in Python's bindings with the workaround mentioned in the issue #18880.

Check the line here:
https://github.com/opencv/opencv/pull/20480/files#diff-e983158fa0a13928ff5c9f999e50f0f36700b98a74ae34f8b925b2e9ff1ce985R328

@alalek
Copy link
Copy Markdown
Member

alalek commented Aug 2, 2021 •
edited
Loading

Got it.
See also this PR #20462 and general issue here: #19091

@JulieBar
Copy link
Copy Markdown
Contributor Author

JulieBar commented Aug 3, 2021

@alalek Do you mean that this test won't be needed when PR #20462 will be merged? If so feel free to close it.

alalek
alalek reviewed Aug 4, 2021
View reviewed changes
alalek
alalek approved these changes Aug 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalek 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 👍

@alalek alalek merged commit 633feda into opencv:3.4 Aug 5, 2021
This was referenced Aug 6, 2021
Closed
Merged
Closed
@alalek alalek mentioned this pull request Oct 5, 2021
Merged
@alalek alalek mentioned this pull request Oct 15, 2021
Merged
@JulieBar JulieBar deleted the lstm_pytest branch November 21, 2021 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

@alalek alalek alalek approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: dnn category: python bindings test

Projects

None yet

Milestone

3.4.16

Development

Successfully merging this pull request may close these issues.

4 participants

@JulieBar @asmorkalov @alalek @JuliaBareeva