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

[G-API]: Add Fluid bitwise operations implementation for (GMat, GScalar) #18257

Merged
merged 7 commits into from
Sep 18, 2020
Merged

[G-API]: Add Fluid bitwise operations implementation for (GMat, GScalar) #18257

merged 7 commits into from
Sep 18, 2020

Conversation

OrestChura
Copy link
Contributor

These changes:

  • add Fluid bitwise operations implementation for (GMat, GScalar) + accuracy and performance tests
    • it is a simple implementation for Fluid (no hal)
    • input Scalar is cast to int in the beginning due to no reason for doing bitwise operations with floating-point numbers
    • tests are enabled/changed to tests operations and operators with a Scalar
    • due to changes for Fluid tests, it was also required to change CPU and GPU tests' instances
      Besides:
  • change Scalar initialization for tests - add floating-point initialization (from tests of core module)
  • avoid the risk of dividing by zero (in accuracy tests, not in performance ones)
    • due to this changes it is possible to run tests for all the operations and operators - they're enabled
    • FIXIT added for avoiding same risks in perf tests
    • changes increased difference of comparison Fluid operations comparing with OpenCV, so their tests' validations are changed

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 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=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2020.3.0:16.04
build_image:Custom Win=openvino-2020.3.0
build_image:Custom Mac=openvino-2020.3.0

test_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

@OrestChura
Added Fluid bitwise with Scalar + acc.tests
6bf3dae 
 - simple loop implementation for Fluid used (no `hal`);
   - `Scalar` is casted to `int` in the beginning
 - tests just modified to work with `Scalar`
 - expected output in operators' tests fixed (operators can't change Mat's depth)
 - `float` `Scalar` `RNG` added, `RNG` reworked (`time` is used now), initialization of test fixtures reworked
   - if input or output is `float` Scalar is initialized by `float`
 - some problems with Fluid/OCV floating-point comparison difference stashed by `AbsSimilarPoints()` usage, FIXME added
 - divide-by-zero is now fixed differently and everywhere
@OrestChura
- Added perf_tests for bitwise_Scalar operations
da08880 
 - due to errors of Fluid floating-point comparison operations, added support of different validation in Cmp perf_tests; added FIXME

 - reworked integral initialization of Scalar
@OrestChura
Copy link
Contributor Author

@anna-khakimova Welcome to review!

@OrestChura
Copy link
Contributor Author

@smirnov-alexey please review

@smirnov-alexey
Copy link
Contributor

Looks good. Thanks for your detailed PR descriptions. Please, assign a reviewer

@OrestChura
Addressing comments
d6f20f4 
 - NULL -> nullptr
 - Scalar convertion moved to the function
 - avoid -> avoiding
@OrestChura
Copy link
Contributor Author

@anna-khakimova Welcome to review!

smirnov-alexey
smirnov-alexey approved these changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@smirnov-alexey smirnov-alexey left a comment

Choose a reason for hiding this comment

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

LGTM

@anna-khakimova
Copy link
Member

Looks good.

@OrestChura
Copy link
Contributor Author

@dmatveev please take a look

dmatveev
dmatveev reviewed Sep 16, 2020
View reviewed changes
modules/gapi/perf/common/gapi_core_perf_tests_inl.hpp
@@ -426,7 +422,7 @@ PERF_TEST_P_(DivRCPerfTest, TestPerformance)
}

// Comparison ////////////////////////////////////////////////////////////
EXPECT_TRUE(cmpF(out_mat_gapi, out_mat_ocv));
// FIXIT unrealiable check: EXPECT_TRUE(cmpF(out_mat_gapi, out_mat_ocv));
Copy link
Contributor

Choose a reason for hiding this comment

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

No new keywords please, we mainly use FIXME or TODO, but not FIXIT.

The idea is to grep by the keyword one day and fix what's found. Not sure anybody remembers about FIXIT at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what has started failing? Does it fail in accuracy tests too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is FIXIT is used all over the perf_tests files, so I just wanted to keep consistency. Should I change all the FIXIT -> FIXME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comparison is unreliable due to potential division by zero. In accuracy tests I fixed it so you can see it in gapi_core_tests_inl.hpp and gapi_operators_tests_inl.hpp (you started a conversation at one of those spots), debugged and tested it to be sure the tests don't fail
It has to be done for performance tests too, but I chose to put this task to backlog as it is not so important to check the accuracy of the performance tests when there are reliable accuracy tests themselves

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR OpenCV divides by zero safely, so we should follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Despite the fact OCV backend follows OpenCV and doesn't cause any accuracy problems, Fluid and GPU backends diverge from OpenCV, so these workarounds are necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

modules/gapi/perf/common/gapi_core_perf_tests_inl.hpp Outdated Show resolved Hide resolved
modules/gapi/src/backends/fluid/gfluidcore.cpp Outdated Show resolved Hide resolved
modules/gapi/src/backends/fluid/gfluidcore.cpp Outdated Show resolved Hide resolved
modules/gapi/src/backends/fluid/gfluidcore.cpp Outdated Show resolved Hide resolved
modules/gapi/test/common/gapi_operators_tests_inl.hpp Outdated Show resolved Hide resolved
modules/gapi/test/cpu/gapi_operators_tests_fluid.cpp Show resolved Hide resolved
@OrestChura
Addressed DM comments
0792e08 
 - refactored convertScalarForBitwise()
 - removed unnecessary braces for switch
@OrestChura
Changed the operators tests
e673c28 
 - switch via `enum` implemented
 - infrastructure for that refactored
dmatveev
dmatveev reviewed Sep 18, 2020
View reviewed changes
Copy link
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@alalek alalek merged commit d1cdef5 into opencv:master Sep 18, 2020
@alalek alalek mentioned this pull request Nov 11, 2021
Open
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
@OrestChura
Merge pull request opencv#18257 from OrestChura:oc/fluid_operator_bit…
23a6aad 
…wise_and_scalar

[G-API]: Add Fluid bitwise operations implementation for (GMat, GScalar)

* Added Fluid `bitwise` with `Scalar` + acc.tests
 - simple loop implementation for Fluid used (no `hal`);
   - `Scalar` is casted to `int` in the beginning
 - tests just modified to work with `Scalar`
 - expected output in operators' tests fixed (operators can't change Mat's depth)
 - `float` `Scalar` `RNG` added, `RNG` reworked (`time` is used now), initialization of test fixtures reworked
   - if input or output is `float` Scalar is initialized by `float`
 - some problems with Fluid/OCV floating-point comparison difference stashed by `AbsSimilarPoints()` usage, FIXME added
 - divide-by-zero is now fixed differently and everywhere

* - Added perf_tests for bitwise_Scalar operations
 - due to errors of Fluid floating-point comparison operations, added support of different validation in Cmp perf_tests; added FIXME

 - reworked integral initialization of Scalar

* Addressing comments
 - NULL -> nullptr
 - Scalar convertion moved to the function
 - avoid -> avoiding

* Addressing comments

* CV_assert -> GAPI_assert

* Addressed DM comments
 - refactored convertScalarForBitwise()
 - removed unnecessary braces for switch

* Changed the operators tests
 - switch via `enum` implemented
 - infrastructure for that refactored
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmatveev dmatveev dmatveev approved these changes

@anna-khakimova anna-khakimova anna-khakimova approved these changes

@smirnov-alexey smirnov-alexey smirnov-alexey approved these changes

Assignees

@dmatveev dmatveev

Labels
category: g-api / gapi
Projects
None yet
Milestone
4.5.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@OrestChura @smirnov-alexey @anna-khakimova @dmatveev @alalek