Performance improvements to isotonic regression.

[robertwb]

No description provided.

[agramfort]

thanks

can you check travis failures?

[ogrisel]

In particular sklearn.tests.test_isotonic.test_isotonic_fit_transform_weight_deprecation is crashing:

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

ERROR: sklearn.tests.test_isotonic.test_isotonic_fit_transform_weight_deprecation

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

Traceback (most recent call last):

File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/build/scikit-learn/scikit-learn/sklearn/tests/test_isotonic.py", line 254, in test_isotonic_fit_transform_weight_deprecation

weight=[1.0/len(y)] * len(y))

File "/home/travis/build/scikit-learn/scikit-learn/sklearn/utils/testing.py", line 152, in assert_warns

result = func(*args, **kw)

File "/home/travis/build/scikit-learn/scikit-learn/sklearn/isotonic.py", line 373, in fit_transform

return self.y_[order_inv]

File "stringsource", line 366, in View.MemoryView.memoryview.__getitem__ (sklearn/_isotonic.c:6508)

File "stringsource", line 351, in View.MemoryView.memoryview.get_item_pointer (sklearn/_isotonic.c:6328)

TypeError: only integer arrays with one element can be converted to an index

[jnothman]

It sounds like y_ is a list (not an array)

On 12 July 2014 19:08, Olivier Grisel [email protected] wrote:

In particular
sklearn.tests.test_isotonic.test_isotonic_fit_transform_weight_deprecation
is crashing:

ERROR: sklearn.tests.test_isotonic.test_isotonic_fit_transform_weight_deprecation

---

Traceback (most recent call last):

File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/build/scikit-learn/scikit-learn/sklearn/tests/test_isotonic.py", line 254, in test_isotonic_fit_transform_weight_deprecation

weight=[1.0/len(y)] * len(y))

File "/home/travis/build/scikit-learn/scikit-learn/sklearn/utils/testing.py", line 152, in assert_warns

result = func(_args, *_kw)

File "/home/travis/build/scikit-learn/scikit-learn/sklearn/isotonic.py", line 373, in fit_transform

return self.y_[order_inv]

File "stringsource", line 366, in View.MemoryView.memoryview.getitem (sklearn/_isotonic.c:6508)

File "stringsource", line 351, in View.MemoryView.memoryview.get_item_pointer (sklearn/_isotonic.c:6328)

TypeError: only integer arrays with one element can be converted to an index

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

[larsmans]

+13kLOC of generated C :(

[larsmans]

It's not guaranteed that either y or weight is contiguous.

[fabianp]

The first patch breaks some user cases by assuming the parameters to be contiguous. @robertwb if you have convincing benchmarks that prove that the use of memory views are worth the trouble of forcing arrays to be contiguous please share, otherwise my opinion is to close this pull request.

The second patch seems a good idea to me. Please submit a separate PR for the second patch if you are interested in getting it in.

[larsmans]

I've already cherry-picked the second commit into master. Closing PR.

[larsmans]

Btw., I was wondering what's causing the speedup, so I inspected the GCC (-std=c99 -O2) assembler output from

void f(double num, double denom, double *solution, size_t n)
{
    for (size_t i = 0; i < n; i++) {
        solution[i] = num / denom;
    }
}

and a version of the above with a ratio cache variable. They're not the same: some instructions are reordered to avoid the division when n==0, presumably to prevent FP exceptions in this case. This may be causing the speedup; I couldn't find a case, even with nested loops as in the IR code, where the compiler actually failed to cache the division.

[robertwb]

Thanks for looking at this.

Yeah, the division thing surprised me as well; your analysis is
interesting. As for memory views, I now see that only the first is
guaranteed to be contiguous. Probably not worth the extra constraint., so
closing this is fine (FWIW, this was inspired by comparison to julia, which
does assume contiguity). The real savings is likely the elimination of a
dereference in the old-style buffers.

On Mon, Jul 14, 2014 at 9:08 AM, Lars Buitinck [email protected]
wrote:

Btw., I was wondering what's causing the speedup, so I inspected the GCC (-std=c99
-O2) assembler output from

void f(double num, double denom, double *solution, size_t n){
for (size_t i = 0; i < n; i++) {
solution[i] = num / denom;
}}

and a version of the above with a ratio cache variable. They're not the
same: some instructions are reordered to avoid the division when n==0,
presumably to prevent FP exceptions in this case. This may be causing the
speedup; I couldn't find a case, even with nested loops as in the IR code,
where the compiler actually failed to cache the division.

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