Object detect module by warmspringwinds · Pull Request #1570 · scikit-image/scikit-image

warmspringwinds · 2015-06-26T15:13:08Z

I marked up the future module.
Changed the mb_lbp function to work faster.

vighneshbirodkar · 2015-06-26T15:45:20Z

skimage/future/__init__.py

I'd call this module detection. Any thoughts from others ?

Definitely start with detect. I'd be fine with detect, detect_object, and probably others.

I agree with @JDWarner, go with detect. It mirrors most of our API, with some well-justified exceptions (filters) and others not so well justified, that we might consider updating for 1.0: segment, restore, view.

Yes, agreed @jni. Simple present verb tense wherever possible.

vighneshbirodkar · 2015-06-26T17:19:07Z

@warmspringwinds This is shaping up really well. Somethings I'd like to see addressed. Some of the old-timers ( if I may say so 😄 ) can help me here

I am not a fan of cpdef at all.
Are we sure we want to include OpenCV's trained XML ? Given that we would replace it with our own later on.
All XML files should do in data and be references by data.xml.faces() or something similar. The way you do it in the test is ugly
If we follow the same XML format as OpenCV should that get a mention ?
Clean up some rough edges, there are places where I can see commented code.

warmspringwinds · 2015-06-26T17:37:19Z

@vighneshbirodkar

Sure. We can make three functions instead of it. If @JDWarner and @jni will help us to find the best solution, it would be great.
I see no problem here. This is a very good file, I tried it and it worked really good. The thing is that I still not getting how OpenCV does the training under the scenes. They made it differently from how it's written in the paper. So it may be the case that we will have to create two different loaders for our file and for the OpenCV file. I am for the support of OpenCV files because it has really performant training script and a lot of ready to use files and people might want to use them.
Yeah, that might be a better idea. I will do it.
Yes, sure. When I am more a less done I will mention this in documentation.
Oh, yeah. Forgot to remove it. Thanks.

vighneshbirodkar · 2015-06-26T17:46:30Z

@warmspringwinds
Regarding 2. I am OK with having 2 files, but we should have only one implementation.

warmspringwinds · 2015-06-26T17:48:15Z

@vighneshbirodkar
We will have only one implementation of training, yes.
But we may support two types of files.

vighneshbirodkar · 2015-06-26T17:49:01Z

@warmspringwinds I mean, we should never have 2 implantations of MB LBP detection.

jni · 2015-06-27T01:03:26Z

skimage/future/objdetect/cascade.pyx

Don't put more than one blank line in a row within methods/functions.

jni · 2015-06-27T05:02:51Z

@vighneshbirodkar @warmspringwinds sorry I don't quite understand the three functions discussion. Our general approach has been to have a cython inner function for performance critical bits, and a pure python wrapper. Is that an option here?

jni · 2015-06-27T05:10:25Z

I also don't know anything about the xml but pre-trained models are in general a very good idea imho. Python in general is a "batteries included" language, so users would be right to expect such convenience. And I rarely favour the creation of a new format, so if something exists that we can parse easily enough, use that.

I hope that helps a bit, sorry if I missed the point but I don't have time right now to delve into the history of this! Thanks both of you for your efforts!

warmspringwinds · 2015-06-27T06:45:04Z

@jni
Sorry, I will elaborate:
We need the function to be cdef, but if we make it so, we won't be able to use it here.

The other way to solve it is to put the function-wrapper in _texture file and make the first function cdef. But in this case the documentation for the function-wrapper won't be correctly generated, because the arguments of a function can't be read from the pyx file. So, I mean that in order for the function documentation to be correctly generated, it has to be in py file.

So, we came up with an ugly solution: to use tree functions: cdef and def in pyx where def wraps the cdef function and another function in py file that wraps def from pyx file.

JDWarner · 2015-06-27T13:02:50Z

This is exactly the use case cpdef was designed for. What are the downsides of letting Cython handle the dual definition for us again, @vighneshbirodkar? There is a clear argument here for simplicity/conciseness?

jni · 2015-06-28T04:18:23Z

skimage/future/objdetect/setup.py

This clearly needs updating.

jni · 2015-06-28T04:54:36Z

@vighneshbirodkar @warmspringwinds I'm afraid I agree with @JDWarner. @vighneshbirodkar, your link specifies the performance tradeoff when subclassing a function, but I don't think it would ever apply here. The answer shows when you wouldn't want to use cpdef, but there is no argument that you should never use cpdef. As @JDWarner says, this is exactly the case where you would want to use it.

At the very least, if we want to use three functions, we would want to see benchmarks showing a clear time advantage to the three function solution.

vighneshbirodkar · 2015-06-28T16:17:21Z

@warmspringwinds I agree that cpdef function has no speed downsides in this case, but Cython does not allow cpdef function to release the GIL. Given how we plan to use prange later, a function acquiring the GIL can turn out to be a huge bottleneck.

jni · 2015-06-29T06:18:19Z

@vighneshbirodkar @warmspringwinds I didn't know about the nogil restriction by cpdef. That would indeed be a compelling reason not to use it.

vighneshbirodkar · 2015-06-30T19:28:09Z

skimage/future/detect_obj/cascade.pyx

Don't harcode, use omp_get_max_threads
See http://www.plutospin.com/files/OpenMP_reference.pdf and http://docs.cython.org/src/userguide/parallelism.html#using-openmp-functions

On my system this evaluates to 4.

@vighneshbirodkar
Yes, on my system it's 4 too. But there is one interesting thing.
When I set the amount of threads to 10 manually, it works faster.

What processor do you have ?

@vighneshbirodkar
Intel(R) Core(TM) i5-4200U CPU @ 1.60GHz

Architecture: x86_64 CPU op-mode(s): 32-bit, 64-bit Byte Order: Little Endian CPU(s): 4 On-line CPU(s) list: 0-3 Thread(s) per core: 2 Core(s) per socket: 2 Socket(s): 1 NUMA node(s): 1 Vendor ID: GenuineIntel CPU family: 6 Model: 69 Stepping: 1 CPU MHz: 759.000 BogoMIPS: 4589.61 Virtualization: VT-x L1d cache: 32K L1i cache: 32K L2 cache: 256K L3 cache: 3072K NUMA node0 CPU(s): 0-3

@vighneshbirodkar
So far, when I test parallel scale computation it's much slower then the parallel image computation. Even though I am not doing any result saving in the scale parallelization part, and the parallel image computation is benchmarked with saving.

@vighneshbirodkar
wow, just played with the schedule parameter. Set it to dynamic, started to work faster even with low amount of threads. I guess I will read more about this thing.

@vighneshbirodkar this occurs because when we spawn threads for each scale they have unequal amount of work (the first scales are the hardest ones) and, therefore, we need to allow them to take up more resources when they need it.

So, we stick with scale threading and dynamic. I hope that my tests were right :)

@warmspringwinds I don't think your PC can run 8 threads.You have 4 CPUs, not 4 cores.
http://ark.intel.com/products/75459/Intel-Core-i5-4200U-Processor-3M-Cache-up-to-2_60-GHz

@vighneshbirodkar Yes, my bad, I made a mistake.

I have a strange behavior in my code.
I did the scale loop, but now every time I get different results and I can't figure out why.
I enclosed my files here, they can be compiled directly:
https://gist.github.com/warmspringwinds/8a1348038df52586905c

Please, have a look if you have time.

@warmspringwinds Check gitter

vighneshbirodkar · 2015-06-30T19:31:45Z

@warmspringwinds This is fantastic, I'm sure with some optimizations, we will have a really fast parallel face detector.

warmspringwinds · 2015-07-06T21:07:10Z

Just faced the issue #867 (comment)
it turned out appveyor also has it.

vighneshbirodkar · 2015-07-07T13:14:50Z

bento.info

@stefanv Your thoughts on the module name ?

arokem · 2016-01-06T01:26:59Z

Seems to compile fine. I get the following error when I run the tests, but
maybe it's unrelated?


======================================================================

ERROR: Failure: AttributeError ('module' object has no attribute
'__pyx_capi__')

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/usr/local/lib/python2.7/site-packages/nose/loader.py", line 418,
in loadTestsFromName

    addr.filename, addr.module)

  File "/usr/local/lib/python2.7/site-packages/nose/importer.py", line 47,
in importFromPath

    return self.importFromDir(dir_path, fqname)

  File "/usr/local/lib/python2.7/site-packages/nose/importer.py", line 94,
in importFromDir

    mod = load_module(part_fqname, fh, filename, desc)

  File "/Users/arokem/source/scikit-image/skimage/future/__init__.py", line
8, in <module>

    from . import graph, detect

  File
"/Users/arokem/source/scikit-image/skimage/future/detect/__init__.py", line
1, in <module>

    from .cascade import Cascade

  File "skimage/future/detect/cascade.pyx", line 1, in init
skimage.future.detect.cascade (skimage/future/detect/cascade.cpp:23779)

    # cython: cdivision=True

AttributeError: 'module' object has no attribute '__pyx_capi__'


----------------------------------------------------------------------

On Tue, Jan 5, 2016 at 4:52 PM, Daniil Pakhomov notifications@github.com
wrote:

@arokem https://github.com/arokem Can you, please, check if my pull
request compiles on your machine with clang?
(I mean without the openMP clang support).
I want to make the compilation possible on the machines without openMP
support (want to check the conditional adding of headers).

—
Reply to this email directly or view it on GitHub
#1570 (comment)
.

warmspringwinds · 2016-01-06T02:33:01Z

@arokem
Thank you so much for you help!

Yeah, it's relevant.

Does it give you the same result when you use your trick with clang-omp for compiling?

stefanv · 2016-01-06T10:47:39Z

Thanks, Ariel, for giving a hand here.

Daniil, I am away until Sunday, but happy to organize an OSX test account for you then.

warmspringwinds · 2016-01-06T23:12:59Z

@stefanv That would be great!

OrkoHunter · 2016-02-06T05:00:28Z

doc/examples/plot_face_detection.py

+# Initialize the detector cascade.
+detector = detect.Cascade(trained_file)
+
+img = data.lena()


We should make sure, lena doesn't go in again.

Daniil hasn't worked on this since his GSoC ended--I imagine he has been quite busy setting up his next research position. Daniil, do you have a bit of time to round this one up?

@stefanv Sure.

I have been mostly working first to make it compile on clang.
Is it still possible to get access to machine with clang?

jni · 2016-06-10T19:47:27Z

I don't know the history of this but @warmspringwinds asked me to test this with CLang and I did and it works fine!

(ana3)
jni@airbears2-10-142-107-176 Thu Jun 09 18:14
 ~/projects/scikit-image (objdetect)$ nosetests skimage/future/detect/tests/test_cascade.py
..
----------------------------------------------------------------------
Ran 2 tests in 2.984s

OK

warmspringwinds · 2016-06-12T23:51:41Z

@JDWarner
@stefanv
@vighneshbirodkar
@jni

I have removed the lena image, updated documentation.
And here are some pictures :P

First image is an .xml file for frontal human face.
Second -- human profile human face.
third -- frontal cat face.

stefanv · 2016-06-13T03:15:22Z

:D

RONNCC · 2016-06-13T04:21:42Z

excited for this :D

jni · 2016-06-21T17:45:27Z

@warmspringwinds huh? Since when do we host notebooks, let alone notebooks in the root dir?

The kitty face detector is awesome.

asnt · 2016-08-30T14:37:54Z

Thank you all for your work on this module.
Just want to report that it builds and runs nicely on my side. (Linux 64bit, Python3.5, OpenMP)

souravsingh · 2017-01-11T08:52:24Z

@warmspringwinds Is the feature ready to be merged in scikit-image? I am really excited to have the feature in scikit-image.

jondo · 2017-11-21T08:43:57Z

What's the state of this? CI already shows some bitrot.

stefanv · 2018-05-29T22:45:20Z

@warmspringwinds This may now be our longest living PR :) Let's revisit:

Is this functionality still relevant for inclusion?
If so, are you willing to carry it the last mile, or should we find some more help?

warmspringwinds · 2018-05-31T05:25:27Z

@stefanv

Yeah, let's finish this up :D
The only issue I had was with the conditional openmp support
to make it compatible with clang:
#1570 (comment)

The detection part is ready and if you could assign someone to help me out with conditional openmp support I think we can finish it up quickly.

I will also update the PR with respect to recent changes in scikit-image in the following days.

Let me know what you think.

glemaitre · 2018-05-31T09:59:40Z

Just jumping in (I might need a cascade classifier somewhere).
I can give an hand if needed. Two questions:

Would it be useful to accept Haar-like features as well as MB-LBP?
Regarding clang-openmp support, what is the reason to not request gcc if people want to build from source. I assume that you are shipping wheel anyway?

jni · 2018-06-01T05:42:34Z

@glemaitre as a heavy mac user, I would be upset if we required gcc for compilation. It's not trivial to get gcc working in macOS. (Actually even getting clang is kind of annoying.) I also like to support different compilers, which might have different optimisation characteristics.

glemaitre · 2018-06-01T08:39:08Z

I also like to support different compilers, which might have different optimisation characteristics.

Good point.

stefanv · 2018-07-26T05:10:26Z

Superceded by #3267.

vighneshbirodkar reviewed Jun 26, 2015
View reviewed changes

jni reviewed Jun 27, 2015
View reviewed changes

skimage/future/objdetect/cascade.pyx Outdated

Copy link
Copy Markdown

Member

jni Jun 27, 2015

Choose a reason for hiding this comment

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

Don't put more than one blank line in a row within methods/functions.

jni reviewed Jun 28, 2015
View reviewed changes

skimage/future/objdetect/setup.py Outdated

Copy link
Copy Markdown

Member

jni Jun 28, 2015

Choose a reason for hiding this comment

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

This clearly needs updating.

vighneshbirodkar reviewed Jun 30, 2015
View reviewed changes

warmspringwinds force-pushed the object_detect_module branch from ba2628b to a5809eb Compare July 6, 2015 16:24

vighneshbirodkar reviewed Jul 7, 2015
View reviewed changes

bento.info

Copy link
Copy Markdown

Contributor

vighneshbirodkar Jul 7, 2015

Choose a reason for hiding this comment

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

@stefanv Your thoughts on the module name ?

OrkoHunter reviewed Feb 6, 2016
View reviewed changes

soupault added the new feature label Feb 6, 2016

lena was removed, documentation was updated

7face0e

object detection ipython notebook was added

059dfc9

soupault mentioned this pull request Feb 13, 2017

Object Detection Process #2502

Closed

warmspringwinds mentioned this pull request Oct 17, 2017

Add Haar-like features #2848

Closed

7 tasks

stefanv added the ⚖️ Needs decision label May 29, 2018

stefanv added this to the 0.15 milestone May 29, 2018

glemaitre mentioned this pull request Jul 14, 2018

Object detector module #3267

Merged

stefanv closed this Jul 26, 2018

grlee77 added 🙏 Feature request and removed type: new feature labels Feb 22, 2022

Uh oh!

Conversation

warmspringwinds commented Jun 26, 2015

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!

vighneshbirodkar commented Jun 26, 2015

Uh oh!

warmspringwinds commented Jun 26, 2015

Uh oh!

vighneshbirodkar commented Jun 26, 2015

Uh oh!

warmspringwinds commented Jun 26, 2015

Uh oh!

vighneshbirodkar commented Jun 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Jun 27, 2015

Uh oh!

jni commented Jun 27, 2015

Uh oh!

warmspringwinds commented Jun 27, 2015

Uh oh!

JDWarner commented Jun 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Jun 28, 2015

Uh oh!

vighneshbirodkar commented Jun 28, 2015

Uh oh!

jni commented Jun 29, 2015

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!

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!

Choose a reason for hiding this comment

Uh oh!

vighneshbirodkar commented Jun 30, 2015

Uh oh!

warmspringwinds commented Jul 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arokem commented Jan 6, 2016

Uh oh!

warmspringwinds commented Jan 6, 2016

Uh oh!

stefanv commented Jan 6, 2016

Uh oh!

warmspringwinds commented Jan 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanv Feb 7, 2016 via email

Choose a reason for hiding this comment

asnt commented Aug 30, 2016 •

edited

Loading

jondo commented Nov 21, 2017 •

edited

Loading