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.

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

More Python implicit numerical conversions and adding .as_array() #30

Closed
dashesy opened this issue Aug 21, 2015 · 4 comments
Closed

More Python implicit numerical conversions and adding .as_array() #30

dashesy opened this issue Aug 21, 2015 · 4 comments

Comments

@dashesy
Copy link
Contributor

dashesy commented Aug 21, 2015

It will be helpful to do some implicit conversions, to allow more seamless integration with Python applications.
For example if I want to use shape predictor with a rectangle object that has 32 bit integer instead of Python's long or double I get this error:

rectangle.__init__(rectangle, numpy.int32, numpy.int32, numpy.int32, numpy.int32) did not match C++ signature:

I noticed I can add e.g. irectangle to rectangles.cpp with a line like bellow to have this also work:

.def(init<boost::int32_t,boost::int32_t,boost::int32_t,boost::int32_t>( (arg("left"),arg("top"),arg("right"),arg("bottom")) ))
  1. I was wondering if boost has a better implicit conversion for some known numeric types. Of course I could just create my rectangle with long(x) but that leaks c++ implementation to Python and the code will look foreign. The reason I had int32 was because it came from another library that is integrated with numpy and if rectangle could just accept the integer type that would have been more Pythonic. This is not just to make dlib work, because it works damn fine already, but a matter of aesthetics and convenience for Python users, and making Python a first-class user of dlib. Even better would be to accept a list (or something iterable) also as rectangle too, and it seems to also be possible with boost.
  2. Some functions return an object that has properties, for example rectangle has top(), bottom(), ... but these objects can be easily converted to a simple numpy array like what OpenCV does. If we could have .to_array() and .to_list() defined for them, it will be less verbose when dealing with those objects than calling those accessors.

Rectangle is just an example, I am sure there are more places that will benefit from relaxing the interface, or adding some helper methods or just adding __repr__.

I have used Cython a lot to build glue layers but boost python integration is new to me, and before looking at dlib I did not even know how well it works already, with docstrings and all!

Do you think in general PRs to do this relaxation are acceptable? or you prefer to have a one-to-one relation to c++ library with API's look-and-feel?

@davisking
Copy link
Owner

I think you can just add a def("init", another_constructor_that_takes_int32) line to the rectangle to add an overload. This way you don't need to introduce more types. Although a better thing to do is probably to add a new construction function that takes 4 boost::python::object instances and appropriately converts them to ints.

You can certainly submit some pull requests that make whatever you think are improvements to the python API and I'll review them. Making the rectangle's constructor take more int types sounds like a good idea. I'm not so sure about the to_array() API though. Allowing someone to reference the elements of a rectangle via [0] or [1] instead of .left() or .top() does not seem like a usability improvement. What use cases does this support? If there are some compelling use cases then sure, but just because OpenCV does it doesn't mean it's a good idea on its own. A lot of the API designs in OpenCV are horrible.

@dashesy
Copy link
Contributor Author

dashesy commented Aug 21, 2015

I also found this answer here for supporting different scalars, will work on PR.

It is not for rectangles actually but returned results that something like to_array() will help. For example after this:

    shape=predictor(g, np.array([long(x),long(y),long(x+w),long(y+h)]))

In order to get the shapes I have to read the number of elements and call a function to get that element, and more accessors to get what is inside the object again.
This is the (somewhat Pythonic) shortest way that I do it now:

    parts = [[shape.part(n).x, shape.part(n).y] for n in range(shape.num_parts)]

But this whole thing screams like C (not even C++), if the returned object could be iterated I could do this (depending on how it can be iterated):

    parts = [shape[i] for i in len(shape)]  # i.e. it has len, so no need to num_parts
    # or
    parts = [a + 1 for a in shape.to_list() if a > 3]  #  i.e. can filter in Pythonic way
    # or 
    parts_str = json.dums(shape.to_list())  # and can send the json over network like any other standard json
   # or 
   plt.plot(shape.to_list())

Or if I just want to find the center of eyes (assuming it is 30 to 40):

    eye_x = np.mean(shape.to_array()[30:40], axis=1)

It helps to have lists or numpy arrays (with missing values as nan) when we need to do some array transformations or matrix multiplication, on the result.

I agree OpenCV API is not the best, for example it would have been useful to override the numpy array and keep color space information somewhere. Pandas does a great job of re-using numpy arrays, so that performance is not hit but extra information is used. That is of course much more work.

In general in Python it will help tremendously if all objects are first-class (or derived from one) because many libraries will work with them out of the box. If they are some sort of container for example, it helps to have __iter__ and indexing, for standard python functions ('map', 'filter', ..) as well as other libraries (scikit-learn, nvd3, ...) to be able to use them.

Of course, the API is most important and should be well thought out, but the current API (for Python) can benefit from some of Python's strengths. Perhaps we can ask more Ptyhon users to see what they think.

@davisking
Copy link
Owner

There are reasonably good docs for boost::python::object here http://www.boost.org/doc/libs/1_59_0/libs/python/doc/tutorial/doc/html/python/object.html.

You could write shape=predictor(g, rectangle(x,y,x+w,y+h)) instead of shape=predictor(g, np.array([long(x),long(y),long(x+w),long(y+h)])) and it would be a lot cleaner.

But yeah, making something that returns a parts array seems like a good idea. You can't turn the entire shape into an array though because it's got a rectangle in it as well, not just the parts. But for example, you could add a parts property that returned a std::vectordlib::point which boost.python will turn into an iterable container (specifically a dlib.points object since there is already a mapping defined of std::vectordlib::points to dlib.points).

It would be less ideal to turn things into python lists since python lists are terribly slow. One of the nice things about boost.python is that it allows you to return actual C++ objects back to python (e.g. std::vectordlib::point) and then when C++ functions are called with those objects you get the actual object back and there is literally no conversion, copying, or any kind of overhead. So everything is very fast. If instead you have an intermediate layer that is copying data from std::vectordlib::points into something else to get it to python and then back to std::vectordlib::points for each call then things are very slow. It might not matter much for these point arrays but in general it's a huge issue, especially for any kind of image processing or numerics. Avoiding copying is important.

@dashesy
Copy link
Contributor Author

dashesy commented Aug 22, 2015

Yes, I meant shape=dlib.predictor(g, dlib.rectangle(long(x),long(y),long(x+w),long(y+h))) the previous one does not work. I had to use long cast because x could be int32.

It is interesting! yes makes sense to keep it as is then. Cython does not have such mechanism.

I think a .to_array() could be better than something like .parts_array though, because 1) it will be created on the fly for those who want the array 2) we could add .to_array() to some other classes too, and api usage will be self documentary. If the class internally is already an array, it could have a .as_array() that will have no copying involved.
In a way to_array() will be just a new representation of the underlying class, while the class will be kept as is. Will add something along those lines for review, and in the meanwhile will learn the glue layer by doing this exercise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants