BUG: make C-implemented root finders work with functools.lru_cache

[jbweston]

Reference issue

Closes #10846.

What does this implement/fix?

C-implemented root finders in scipy.optimize raise SystemError when passed a function wrapped with functools.lru_cache. This is because when calling C-implemented root finders we create the tuple of arguments to pass to the underlying function once, and modify only the first entry on subsequent calls. This breaks when the argument tuple is stored internally by functools.lru_cache.

My fix is to instead construct a new argument tuple each time the underlying function is called.

As this change has the potential to slow down the root finders I ran the following benchmark:

from scipy.optimize.zeros import bisect

def f(x):
    return x

# The slowest root finder
%timeit bisect(f, -1, 1 + 1/3)

on master this gives:
4.78 µs ± 277 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
on this branch this gives:
5.18 µs ± 204 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

which represents an 8% slowdown. I think that this is pretty much an upper bound on the slowdown, as the function we're finding the root of is trivial, and this is using the most naive root-finder (i.e. it does the least amount of work per function call).

Additional information

This bug is present in older versions of Scipy, but I have applied the fix on top of master. I could not find any information on Scipy's policy regarding how far back bugfixes should be applied. Let me know if I should apply this fix to one of the maintenance/* branches instead.

[jbweston]

Doc building failures don't seem to have anything to do with this PR.

[ewmoore]

I think the bulk of the code you’ve added can probably be replaced with a call to PySequence_Concat.

[jbweston]

@ewmoore the last commit uses PySequence_Concat, however the timings have deteriorated.

Running the above benchmark with this new commit added gives:
7.98 µs ± 106 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
which is a much larger slowdown.

[ewmoore]

Well never mind then.

…

On Tue, Oct 1, 2019 at 12:52 PM Joseph Weston ***@***.***> wrote: @ewmore the last commit uses PySequence_Concat, however the timings have deteriorated. Running the above benchmark with this new commit added gives: 7.98 µs ± 106 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) which is a much larger slowdown. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10891?email_source=notifications&email_token=AARVDELGZI24A3KXPY63VIDQMN53ZA5CNFSM4I4KIRXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAB6MII#issuecomment-537126433>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARVDEL76A3AL6ZUQK7TROLQMN53ZANCNFSM4I4KIRXA> .

[jbweston]

I just removed the commit that used PySequence_Concat.

[jbweston]

@pv I think this fix should also be backported to previous scipy versions, but I don't know scipy's policy on providing bugfixes for older versions.