Alternative query integral image

[Aierie]

Found a small kink in the goal generator - including 0 is inefficient
(and creates garbage with my subfunctions)

Tried cdef subfunctions to improve speed!

Also added a unit test file (WIP, not sure of the best way to handle Cython in unit testing).

I'm not very experienced in package creation, so I'm passing this over for a quick look. If it looks okay I'll give it a go!

[jcfr]

Thanks for the contribution.

Since we recently transition to pytest for writing our tests.

Would it be possible to:

• rename Unit_tests.py into test_hits.py (or similar name)
• have a single function for each new tests added to the test_hits.py file (instead of creating a hierarchy of class).

To execute the test, run pytest from the top-level folder.

the best way to handle Cython in unit testing

Waiting, we make a decision on #424 , you have to locally regenerate the file wordcloud/query_integral_image.c file running cython wordcloud/query_integral_image.pyx

Then, testing will work as expected because we run pip install -e . before executing the tests. This ensure the module is compiled and available in the source tree.

[Aierie]

Thanks @jcfr ! I'll give it a go.

[jcfr]

Thanks for the follow up.

To address the coding style issue locally, you can run:

flake8

in the top level directory of the project.

[Aierie]

Hmm. Seems like there's still a fair amount of work to do with this - I'll download CircleCI and try and work on it! Thanks for the suggestions @jcfr ! Closing for now.

[jcfr]

Sounds good. It may also be worth sharing some timing result. That would help @amueller assess if moving forward with the approach is sensible.

[Aierie]

@amueller

With this setup:

integral_image = np.zeros((400, 400), dtype=np.uint32)
size_x = 10
size_y = 10
random_state = random.Random()

Running timeit (10000 times) yields:
original query_integral_image: 18.97049316452103s
alternative: 15.947954525594469

%lprun -f query_integral_image -f alternative alternative(integral_image, size_x, size_y, random_state); query_integral_image(integral_image, size_x, size_y, random_state)

yields:
original query_integral_image: 0.219951 s
alternative: 0.00224246 s

Not sure about the dramatic difference in lprun but the jupyter notebook I used is here:
https://github.com/Aierie/testing_query_integral_image-alternative.git

I originally was attempting to implement an array storage of hit coordinates but found that this was reasonably fast!

[amueller]

@Aierie cool! Can you maybe run the timing using an actual word-cloud?

[Aierie]

I've tried going with an actual wordcloud, and apparently it actually makes it slower.

original: 527 ms ± 3.19 ms per loop (mean ± std. dev. of 7 runs, 1000 loops each)
alternative: 562 ms ± 2.07 ms per loop (mean ± std. dev. of 7 runs, 1000 loops each)

I don't really understand why - any idea? I suppose this means that it's not too viable an approach.

    cdef int goal = random_state.randint(0, hits)
    hits = 0
    for i in xrange(x - size_x):
        for j in xrange(y - size_y):
            area = integral_image[i, j] + integral_image[i + size_x, j + size_y]
            area -= integral_image[i + size_x, j] + integral_image[i, j + size_y]
            if not area:
                hits += 1
                if hits == goal:
                    return i, j

I found this while working through the code:
Could consider changing
cdef int goal = random_state.randint(0, hits) to cdef int goal = random.randint(1, hits)
because getting goal = 0 automatically returns (0, 0) and affects sizing and placement, and search efficiency/termination. Should I open a pull for this?

Edit: returns (0, 0) assuming the area is occupied

[jcfr]

Could consider changing
cdef int goal = random_state.randint(0, hits) to cdef int goal = random.randint(1, hits)
because getting goal = 0 automatically returns (0, 0) and affects sizing and placement, and search efficiency/termination. Should I open a pull for this?

Did you get a chance to do some timing comparison within and without this optimization ?

[Aierie]

Sorry, I'm confusing myself. 😬 i'm mixing up my code results with the original. With the original the worst that could happen is an overlap. I need a break I think 😂

[Aierie]

Thanks for the tips guys! @jcfr @amueller Will be back when I'm better prepared!