Skip to content

feat: update conversion logic for std::vector<T> in Python bindings#20618

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
VadimLevin:dev/vlevin/fix-vector-conversion
Sep 1, 2021
Merged

feat: update conversion logic for std::vector<T> in Python bindings#20618
opencv-pushbot merged 1 commit intoopencv:3.4from
VadimLevin:dev/vlevin/fix-vector-conversion

Conversation

@VadimLevin
Copy link
Copy Markdown
Contributor

@VadimLevin VadimLevin commented Aug 27, 2021

Resolves #18412, resolves #14636

PyObject* to std::vector<T> conversion logic:

  • If user passed Numpy Array
    • If array is planar and T is a primitive type (doesn't require constructor call) that matches with the element type of array, then
      copy element one by one with the respect of the step between array elements. If compiler is lucky (or brave enough) copy loop can be vectorized.
      For classes that require constructor calls this path is not possible, because we can't begin an object lifetime without hacks.
    • Otherwise fall-back to general case
  • Otherwise - execute the general case:
    If PyObject* corresponds to Sequence protocol - iterate over the sequence elements and invoke the appropriate pyopencv_to function.

std::vector<T> to PyObject* conversion logic:

  • If std::vector<T> is empty - return empty tuple.
  • If T has a corresponding Mat DataType than return
    Numpy array instance of the matching dtype e.g. std::vector<cv::Rect> is returned as np.ndarray of shape Nx4 and
    dtype=int.
    This branch helps to optimize further evaluations in user code.
  • Otherwise - execute the general case:
    Construct a tuple of length N = std::vector::size and insert elements one by one.

Unnecessary functions were removed and code was rearranged to allow compiler select the appropriate conversion function specialization.

Possible may resolve #10158 if DataType for DMatch deprecation will be revoked.

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

alalek
alalek reviewed Aug 27, 2021
View reviewed changes
@VadimLevin VadimLevin force-pushed the dev/vlevin/fix-vector-conversion branch 3 times, most recently from 70c7c64 to d4f18b8 Compare August 28, 2021 18:48
@VadimLevin VadimLevin mentioned this pull request Aug 28, 2021
Merged
6 tasks
@VadimLevin
Copy link
Copy Markdown
Contributor Author

VadimLevin commented Aug 28, 2021 •
edited
Loading

Failed test in aruco is fixed in opencv/opencv_contrib#3031 :

-        ids = np.array([[elem] for elem in range(17)])
-        rev_ids = np.array(list(reversed(ids)))
+        ids = np.arange(17)
+        rev_ids = ids[::-1]

In original test - 2d array passed to std::vector<int> which is wrong in my opinion.
The error is have exact problem in user code, so user can easily troubleshot it:

Traceback (most recent call last):
  File "/build/precommit_linux64/3.4/opencv_contrib/modules/aruco/misc/python/test/test_aruco.py", line 24, in test_idsAccessibility
    board.ids = rev_ids
TypeError: Can't parse 2D array as 'value' vector argument

@VadimLevin VadimLevin force-pushed the dev/vlevin/fix-vector-conversion branch 3 times, most recently from ebdc183 to 03c25a2 Compare August 30, 2021 08:28
@VadimLevin VadimLevin requested a review from alalek August 31, 2021 09:08
alalek
alalek reviewed Aug 31, 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.

Well done!

modules/core/include/opencv2/core/bindings_utils.hpp Outdated
if (len > 0)
{
RNG rng(12345);
cv::Mat tmp(static_cast<int>(len), 1, CV_32SC4);
Copy link
Copy Markdown
Member

@alalek alalek Aug 30, 2021

Choose a reason for hiding this comment

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

avoid using of unnecessary cv:: in OpenCV source code

modules/core/src/bindings_utils.cpp Outdated
}

template <class T, class Formatter>
String dumpVector(const std::vector<T>& vec, Formatter format)
Copy link
Copy Markdown
Member

@alalek alalek Aug 30, 2021

Choose a reason for hiding this comment

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

static or static inline?

@AleksandrPanov AleksandrPanov mentioned this pull request Aug 31, 2021
Open
4 tasks
@VadimLevin
feat: update conversion logic for std::vector<T> in Python bindings
16b9514 
`PyObject*` to `std::vector<T>` conversion logic:
- If user passed Numpy Array
  - If array is planar and T is a primitive type (doesn't require
    constructor call) that matches with the element type of array, then
    copy element one by one with the respect of the step between array
    elements. If compiler is lucky (or brave enough) copy loop can be
    vectorized.
    For classes that require constructor calls this path is not
    possible, because we can't begin an object lifetime without hacks.
  - Otherwise fall-back to general case
- Otherwise - execute the general case:
  If PyObject* corresponds to Sequence protocol - iterate over the
  sequence elements and invoke the appropriate `pyopencv_to` function.

`std::vector<T>` to `PyObject*` conversion logic:
- If `std::vector<T>` is empty - return empty tuple.
- If `T` has a corresponding `Mat` `DataType` than return
  Numpy array instance of the matching `dtype` e.g.
  `std::vector<cv::Rect>` is returned as `np.ndarray` of shape `Nx4` and
  `dtype=int`.
  This branch helps to optimize further evaluations in user code.
- Otherwise - execute the general case:
  Construct a tuple of length N = `std::vector::size` and insert
  elements one by one.

Unnecessary functions were removed and code was rearranged to allow
compiler select the appropriate conversion function specialization.
@VadimLevin VadimLevin force-pushed the dev/vlevin/fix-vector-conversion branch from 03c25a2 to 16b9514 Compare September 1, 2021 10:00
alalek
alalek approved these changes Sep 1, 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 👍

@opencv-pushbot opencv-pushbot merged commit 6625810 into opencv:3.4 Sep 1, 2021
@VadimLevin VadimLevin linked an issue Sep 1, 2021 that may be closed by this pull request
SystemError: <built-in function NMSBoxes> returned NULL without setting an error #18412
Closed
4 tasks
alalek
alalek reviewed Sep 1, 2021
View reviewed changes
@alalek alalek mentioned this pull request Sep 1, 2021
Merged
@alalek alalek mentioned this pull request Oct 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

Assignees

@alalek alalek

Labels

affected: 3.4 category: python bindings feature test

Projects

None yet

Milestone

3.4.16

Development

Successfully merging this pull request may close these issues.

SystemError: <built-in function NMSBoxes> returned NULL without setting an error

3 participants

@VadimLevin @alalek @opencv-pushbot