Fix implicit conversion from array to scalar in python bindings by VadimLevin · Pull Request #15915 · opencv/opencv

VadimLevin · 2019-11-15T13:25:58Z

This pullrequest changes

numpy array was implicitly converted by PyArg_ParseTupleAndKeywords format string. After removal possible conflicting types from simple_argtype_mapping pyopencv_to parsers for them are generated, where additional checks are done, so correct overload is chosen.

resolves #11426

Fix implicit conversion of numpy array with 1 element to scalar
Add tests for norm

asmorkalov · 2019-11-16T13:41:00Z

@VadimLevin The patch introduce 1000+ compile warnings. Please take a look: https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/23624/steps/compile%20release/logs/warnings%20%281490%29

VadimLevin · 2019-11-18T10:29:33Z

@alalek Can you have a look to this change?

alalek

Bindings changes look good to me.

Test looks big, but it is still fast (~0.05 sec). Also I would note that this problem is not related to norm() only, so more general checks can be added later.
Large part of these Python tests does not test accuracy in all flavors - these python tests should test bindings generator at first.

modules/python/test/test_norm.py

alalek · 2019-11-18T13:36:54Z

modules/python/src2/gen2.py

+    "int": ArgTypeInfo("int", FormatStrings.int, "0", True),
+    "float": ArgTypeInfo("float", FormatStrings.float, "0.f", True),
+    "double": ArgTypeInfo("double", FormatStrings.double, "0", True),
+    "c_string": ArgTypeInfo("char*", FormatStrings.string, '(char*)""')


Looks good to me.

modules/python/test/test_norm.py

alalek · 2019-11-19T11:04:08Z

This change may break users code, so we need to ensure that impact would be low.

So we need tests. We can start with bindings arguments handling/conversions.
We still don't have them 😞

Initial part of tests is here: alalek@pr15915_r
(some cases are failed with this patch - due bugs in pyopencv_to<int>() implementation)

- Introduce ArgTypeInfo namedtuple instead of plain tuple. If strict conversion parameter for type is set to true, it is handled like object argument in PyArg_ParseTupleAndKeywords and converted to concrete type with the appropriate pyopencv_to function call. - Remove deadcode and unused variables. - Fix implicit conversion from numpy array with 1 element to scalar - Fix narrowing conversion to size_t type.

- Introduce ArgTypeInfo namedtuple instead of plain tuple. If strict conversion parameter for type is set to true, it is handled like object argument in PyArg_ParseTupleAndKeywords and converted to concrete type with the appropriate pyopencv_to function call. - Remove deadcode and unused variables. - Fix implicit conversion from numpy array with 1 element to scalar - Fix narrowing conversion to size_t type.· - Enable tests with wrong conversion behavior - Restrict passing None as value - Restrict bool to integer/floating types conversion

alalek

Well done!

alalek · 2019-12-03T14:21:41Z

modules/python/src2/cv2.cpp

-    int _val = PyObject_IsTrue(obj);
-    if(_val < 0)
+    }
+    if (obj == Py_None)


Currently "None" may be used for parameters with default values.
Lets try to keep this behavior.
However, it is not Pythonic and should be deprecated/removed (it is not good for "strict" overloads selector).

Perhaps:

we can extent ArgInfo with "hasDefaultValue" field.

if (obj == Py_None && info.hasDefaultValue) { return true; // keep default value from declaration } else { failmsg... }

I've got your point and agree with it. I will add default value check for None as a separate PR.

alalek · 2019-12-11T14:29:56Z

modules/python/src2/cv2.cpp

+
+// Python definition alias to prevent define collisions
+#if defined(PY_LONG_LONG)
+#define CV_HAVE_LONG_LONG 1


Looks like it is not used.

Thanks for catch it up. I've changed check for appropriate type. To be removed.

- Remove unused macro - Add better conversion for types to numpy types descriptors - Add argument name to fail messages - NoneType treated as a valid argument. Better handling will be added as a standalone patch

modules/python/src2/cv2.cpp

- If signed integer is positive it can be safely converted to unsigned - Add check for plain python 2 objects - Add check for numpy scalars - Add simple type_traits implementation for better code style

- Move type_traits to separate header

alalek · 2019-12-23T20:59:41Z

modules/python/src2/cv2.cpp

+        // According to the numpy documentation:
+        // There are 21 statically-defined PyArray_Descr objects for the built-in data-types
+        PyArray_Descr* to = getNumpyTypeDescriptor<T>();
+        if (canBeSafelyCasted<T>(obj, to))


canBeSafelyCasted<T>

This looks over complicated. Code doesn't use all 21 types, because it is OpenCV code (I see only 3 specializations for parseNumpyScalar - size_t, float, double).

I suggest to remove "stdx" replacement and just write these specializations (or overloads).

The underlying idea of this generalized approach is to reduce code repetition and have a set well tested functions as building blocks. Basically, it is possible to cover all primitive types by 3 general functions:

For signed integral types;

For unsigned integral types;

For floating point types.

And 1 specialized function for size_t.

After primitive all primitive types are covered by this set of functions, it is possible to cover other cases with their combination.
For instance, vector<type> can be parsed as a sequence without fixed size, cv::Size can be parsed as a sequence with size 2. To parse a single element generalized function should be called.

If a new type (e.g. int64_t can not be parsed now) will be added, it automatically be covered by signed integral parser, so the contributor should provide only tests for it.

These functions are not introduced in the current PR, but will be in further.

If you are against this idea, I'm OK to rewrite this part with parsers specializations.

Vector conversions is another story. I still didn't get problems with them.

If a new type

Set of types for bindings should be limited to cover other than PYthon languages too.

Lets keep patch as much simple as possible.

@alalek
Added some test to illustrate current problem with sequential types parsing (such as vector or Size (sequence of size 2)):
VadimLevin@07bdc4a

For vector<int> skipped test either fails with some inner error or perform narrowing conversion (double -> int) which we want to eliminate with strong type check.

For cv::Size it simply rejects everything except tuple.

So the main goal of the underlying idea, that I tried to describe in the previous comment, is elimination of confusing and non-intuitive conversion behavior.
Implementation with templates may be a bit complicated, but in my opinion has more advantages rather disadvantages.

asmorkalov · 2020-01-10T06:15:26Z

@alalek @VadimLevin Do you have any conclusion on the patch?

- Made canBeSafelyCasted specialized only for size_t, so type_traits header became unused and was removed. - Added clarification about descriptor pointer

asmorkalov · 2020-01-13T11:39:02Z

@alalek Please take a look. Vadim reduced the patch as discussed offline.

alalek

Thank you 👍

alalek reviewed Nov 18, 2019

View reviewed changes

VadimLevin mentioned this pull request Nov 20, 2019

Tests for argument conversion of Python bindings generator #15955

Merged

VadimLevin force-pushed the dev/norm_fix branch 2 times, most recently from ce183ba to 68c9757 Compare December 2, 2019 06:35

VadimLevin force-pushed the dev/norm_fix branch from 68c9757 to 9ea4b7c Compare December 2, 2019 07:26

VadimLevin added 2 commits December 2, 2019 13:36

Add PyIntType support for Python 2

bb1da36

Remove possible narrowing conversion of size_t

5143e80

alalek reviewed Dec 11, 2019

View reviewed changes

Bindings conversion update

795a597

- Remove unused macro - Add better conversion for types to numpy types descriptors - Add argument name to fail messages - NoneType treated as a valid argument. Better handling will be added as a standalone patch

VadimLevin force-pushed the dev/norm_fix branch from 2ebdaae to 795a597 Compare December 12, 2019 09:02

Add descriptor specialization for size_t

aff0cfe

VadimLevin force-pushed the dev/norm_fix branch from 93a5b92 to aff0cfe Compare December 13, 2019 13:02

alalek reviewed Dec 16, 2019

View reviewed changes

modules/python/src2/cv2.cpp Show resolved Hide resolved

Add check for signed to unsigned integer conversion safety

74144f8

- If signed integer is positive it can be safely converted to unsigned - Add check for plain python 2 objects - Add check for numpy scalars - Add simple type_traits implementation for better code style

VadimLevin force-pushed the dev/norm_fix branch 2 times, most recently from 67c1dbd to fe7fdb4 Compare December 17, 2019 11:30

Resolve type "overflow" false negative in safe casting check

abb0129

- Move type_traits to separate header

VadimLevin force-pushed the dev/norm_fix branch from fe7fdb4 to abb0129 Compare December 17, 2019 12:15

Add copyright message to type_traits.hpp

2bf5126

alalek reviewed Dec 23, 2019

View reviewed changes

Limit conversion scope for integral numpy types

fa3d996

- Made canBeSafelyCasted specialized only for size_t, so type_traits header became unused and was removed. - Added clarification about descriptor pointer

alalek approved these changes Jan 13, 2020

View reviewed changes

alalek merged commit 31289d2 into opencv:3.4 Jan 13, 2020

alalek mentioned this pull request Jan 14, 2020

Merge 3.4 #16346

Merged

VadimLevin mentioned this pull request Mar 18, 2020

dnn: expose only float variant of NMSBoxes for bindings #16828

Merged

6 tasks

alalek mentioned this pull request Dec 18, 2020

fix: conversion to string in python bindings #19137

Merged

6 tasks

Burnside999 mentioned this pull request Jul 19, 2024

Slow speed using cv2.norm(img1,img2) with opencv > 4.2.0.34 #25928

Closed

4 tasks

Uh oh!

Conversation

VadimLevin commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This pullrequest changes

Uh oh!

asmorkalov commented Nov 16, 2019

Uh oh!

VadimLevin commented Nov 18, 2019

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalek commented Nov 19, 2019

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VadimLevin Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Jan 10, 2020

Uh oh!

asmorkalov commented Jan 13, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VadimLevin commented Nov 15, 2019 •

edited

Loading

VadimLevin Dec 24, 2019 •

edited

Loading