Dynamically create scipy.special.cython_special signatures

[brandonwillard]

This PR updates some of the scipy.special.cython_special signatures for SciPy 1.6.2 (and possibly lower).

This approach doesn't use the generated signatures, since those appear to represent only a snapshot of a particular version. Instead, the few signatures that vary between versions were updated directly.

A more dynamic approach would be better (e.g. determine the signatures when the module is loaded), but I'm not familiar enough with the project to say much about that. At the very least, it seems like the generated signature snapshots could be made to include the SciPy version numbers in some way.

Regardless, I do not believe that pinning the version of SciPy is a good approach (i.e. #37), because it unnecessarily restricts the applicability of the project entirely—especially since it took very little time to update the signatures so that they work across at least the three SciPy versions tested.

[brandonwillard]

I just added a commit that creates the maps when the sub-package numba_scipy.special.signatures loads. The code is copied straight from scripts.generate_special_function_declarations and it appears to work.

[esc]

@brandonwillard thank you for submitting this! How does this relate to #37 ? Is it an alternative approach?

It seems like all Github Actions have passed here, so that is a good indication. Also, the GithubActions pin Scipy to 1.6 :

https://github.com/numba/numba-scipy/blob/master/.github/workflows/test_and_deploy.yml#L33

[esc]

Some more questions I have:

• Can we remove scripts.generate_special_function_declarations?
• Do we want to update CI to include tests against multiple versions of Scipy?

[esc]

More questions:

• Do we believe that the test coverage for this is sufficient?
• Do we want to generate the signatures for everything on import? Or lazily, as and when a function is called?

[brandonwillard]

• Can we remove scripts.generate_special_function_declarations?

I didn't remove the script because it also generates the docs.

• Do we want to update CI to include tests against multiple versions of Scipy?

I just added a commit for that.

• Do we believe that the test coverage for this is sufficient?

The tests do seem to be testing all the bindings automatically. Aside from the three XFAILS, that seems to be about as good as it gets.

• Do we want to generate the signatures for everything on import? Or lazily, as and when a function is called?

Loading everything at import time seems perfectly fine; the cost of generating the dicts is rather low and it keeps the implementation simple and manageable.

[esc]

• Can we remove scripts.generate_special_function_declarations?

I didn't remove the script because it also generates the docs.

👌

• Do we believe that the test coverage for this is sufficient?

The tests do seem to be testing all the bindings automatically. Aside from the three XFAILS, that seems to be about as good as it gets.

👌

• Do we want to generate the signatures for everything on import? Or lazily, as and when a function is called?

Loading everything at import time seems perfectly fine; the cost of generating the dicts is rather low and it keeps the implementation simple and manageable.

👌

[esc]

• Do we want to update CI to include tests against multiple versions of Scipy?

I just added a commit for that.

I think (but do correct. me if I am wrong), now that we generate the signatures at runtime, we may need to refactor the Github Actions CI. BTW, don't bother futzing about with the Azure CI config, it's basically "scheduled for removal" anyway. I think the SciPy requirement musn't be enforced during build, but rather during test. So for example, to make the matrix span the test_pyver tasks rather than the build tasks? I see the testing is done only on Scipy 1.3.2 at the moment. Does that make sense?

[brandonwillard]

I see the testing is done only on Scipy 1.3.2 at the moment.

Sorry, that was just a typo in the commit; I've updated it now.

[esc]

I see the testing is done only on Scipy 1.3.2 at the moment.

Sorry, that was just a typo in the commit; I've updated it now.

Thank you, I checked and the constraints seem to work well, obtaining whatever SciPy version is available for the given Python version. I think it's great that we don't need to pin anything here.

[brandonwillard]

The issue was, that one developer had a pre-rebase version, the commit hash was included in the version string. Later on someone else cloned the repo and fetched a post-rebase version and was unable to find the pre-rebase commit hash which led to confusion.

I don't quite understand, but I'll add extra commits, if that helps. I imagine we'll need to squash all the noisy commits, if so, though.

[esc]

The issue was, that one developer had a pre-rebase version, the commit hash was included in the version string. Later on someone else cloned the repo and fetched a post-rebase version and was unable to find the pre-rebase commit hash which led to confusion.

I don't quite understand, but I'll add extra commits, if that helps. I imagine we'll need to squash all the noisy commits, if so, though.

yes, we can squash and cleanup as soon as we are all happy and before the merge if you like.

[adeak]

@adeak anything else come to mind?

Well to be fair the only technical matter here that I understand is the regex, and yes, escaping the parens and adding the r should suffice in that regard.

You did mention something about the docs and I can see https://numba-scipy.readthedocs.io/en/latest/user/index.html is broken (or just empty on purpose?). If there's a way to build that locally (I'd read the docs but... well :D) we can check if that's fixed as well. Or if that's broken for a different reason then this is irrelevant here.

[esc]

@adeak anything else come to mind?

Well to be fair the only technical matter here that I understand is the regex, and yes, escaping the parens and adding the r should suffice in that regard.

You did mention something about the docs and I can see https://numba-scipy.readthedocs.io/en/latest/user/index.html is broken (or just empty on purpose?). If there's a way to build that locally (I'd read the docs but... well :D) we can check if that's fixed as well. Or if that's broken for a different reason then this is irrelevant here.

I think that page is empty by design, which is probably a different issue.

What I mean was the auto-generated API docs at:

https://numba-scipy.readthedocs.io/en/latest/reference/special.html

Since the code in generate_special_function_declarations was also touched we need to double check, that the API docs can still be generated before we release. I think, fixing the paren escape will suffice. I can do this before I release, no problem.

[adeak]

I see, thanks!

So next issue: is it expected that only scalars can be passed to the jitted functions?

import numpy as np
from numba import njit
from scipy import special

x = np.linspace(-10, 10, 1000)

@njit
def jitted_j0(x):
    # res = special.j0(x)  # breaks
    res = special.j0(x[0])  # works
    return res

print(jitted_j0(x))

[brandonwillard]

So next issue: is it expected that only scalars can be passed to the jitted functions?

Good question. Was it ever able to handle those? I don't see where the signatures for array inputs were handled before.

[esc]

I see, thanks!

So next issue: is it expected that only scalars can be passed to the jitted functions?

import numpy as np
from numba import njit
from scipy import special

x = np.linspace(-10, 10, 1000)

@njit
def jitted_j0(x):
    # res = special.j0(x)  # breaks
    res = special.j0(x[0])  # works
    return res

print(jitted_j0(x))

Oh dear... In the interest of noise reduction, I would suggest to complete the current PR first and then open the reproducer your reported as a new issue, how does that sound?

[esc]

So next issue: is it expected that only scalars can be passed to the jitted functions?

Good question. Was it ever able to handle those? I don't see where the signatures for array inputs were handled before.

Yeah, I would assume that this never worked as there are no tests for it...

[brandonwillard]

Oh dear... In the interest of noise reduction, I would suggest to complete the current PR first and then open the reproducer your reported as a new issue, how does that sound?

Definitely. I would suggest the same regarding the documentation. At this point, the project is completely broken, so it might be best to just get it working in some fashion first.

[adeak]

Oh dear... In the interest of noise reduction, I would suggest to complete the current PR first and then open the reproducer your reported as a new issue, how does that sound?

Right, we only have scalar signatures! I don't mean to derail the PR at all, I just couldn't figure out if this was relevant. It clearly isn't; so do ignore it. I'll open an issue to keep track of this feature request.

[esc]

Oh dear... In the interest of noise reduction, I would suggest to complete the current PR first and then open the reproducer your reported as a new issue, how does that sound?

Right, we only have scalar signatures! I don't mean to derail the PR at all, I just couldn't figure out if this was relevant. It clearly isn't; so do ignore it. I'll open an issue to keep track of this feature request.

Thank you! Numba loves loops so looping over the array should be an acceptable workaround for the time being if you really need it.

[adeak]

Yeah, I'm just prodding it to see what works. I know loops are fine with numba, I just figured I'd try. And if I'm being honest I don't even use scipy.special functions, I just found numba-scipy thanks to a question on Stack Overflow 😅

[esc]

I still think more tests should be added for the utility functions introduced. But we can add those with a later PR.

[stuartarchibald]

Thanks for implementing this @brandonwillard. Thanks for the discussion/review/comments @adeak and @esc. I've taken a look at the patch and suggest the following:

• As it resolves the blocking issue relating to signatures/cython bindings and reviewers are happy then this should be merged.
• Prior to release, consider setting a maximum supported SciPy version so as ensure that if the regex parsing scheme is invalidated by an upstream change then this package doesn't immediately break.
• Prior to release, perhaps add some acceptable failure modes to the utility functions along with appropriate tests as suggested by @esc.

Thanks again!

[brandonwillard]

• Prior to release, consider setting a maximum supported SciPy version so as ensure that if the regex parsing scheme is invalidated by an upstream change then this package doesn't immediately break.

Bounding the SciPy version introduces a difficult trade-off for this package, because, in some cases, users will have to decide between using this package or the latest version of SciPy. Obviously that decision will only be encountered as often as this package does/doesn't get updated, but I think it's safer to just allow this package to fail "gracefully" in those cases.

If by failing gracefully we mean that certain signatures may work, while others don't, then this approach will always be better than not allowing anything to work on newer version of SciPy.

[esc]

• Prior to release, consider setting a maximum supported SciPy version so as ensure that if the regex parsing scheme is invalidated by an upstream change then this package doesn't immediately break.

Bounding the SciPy version introduces a difficult trade-off for this package, because, in some cases, users will have to decide between using this package or the latest version of SciPy. Obviously that decision will only be encountered as often as this package does/doesn't get updated, but I think it's safer to just allow this package to fail "gracefully" in those cases.

If by failing gracefully we mean that certain signatures may work, while others don't, then this approach will always be better than not allowing anything to work on newer version of SciPy.

It really depends. In Numba, we now have the policy to disallow any future version of Python (e.g. 3.10). This means, instead of obscure error message and lot's of debugging by the maintainers, users receive an error message that states very clearly that this version of Python is not supported.

For this package I would ask the question, how often does SciPy receive releases? Can we do anything to check against SciPy nightlies (if such a thing exists) to ensure SciPy isn't breaking this package (this would allow an early warning system)? Can we release in lockstep with SciPy? And can we have a way for expert users to "ignore" the SciPy dependency pin?

I would much rather operate in a defensive regime in this case and open up new SciPy releases after they have been confirmed to work with/for numba-scipy.

[esc]

In any case, the discussion if we should pin to a max SciPy version or not can be discussed in a new issue on the pull-request that implements the pin.

For the time being, thank you @brandonwillard for proposing the fix and thank you @adeak for reviewing it! Thank you also @stuartarchibald for the final review and advice. I will now merge this PR.