-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Allow axis and keepdims arguments to be passed to scipy.linalg.norm. #4390
ENH: Allow axis and keepdims arguments to be passed to scipy.linalg.norm. #4390
Conversation
cf45ce6
to
d8df41f
Compare
I'm not sure if I'm really happy with functionality that depends on a dependency's version to work or not work. This is very confusing for users wanting to control their dependencies. It might be a slightly better idea to disable the functionality depending on whether the NumPy that is imported by SciPy has the desired kwargs, but that would drag in a lot of Python inspection magic. |
@larsmans Yes, it is an odd idea, but not having the keywords available in both versions is really not ideal. It would be nice to have some sort of workaround. My understanding of the function here is that it is a wrapper of the numpy function with an additional special case that calls BLAS for better stability to prevent floating point overflow. Understood in that light, it makes sense to pass along whatever arguments are possible. If nothing else, we should make that clearer in the docs so that newcomers will know where to get the functionality they need. |
Maybe the right thing to do is to copy the numpy implementation and retrofit the BLAS special case into it? |
I'm OK with optional features loudly failing if the numpy version is too old, but I think the implementation in this PR will silently ignore axis and keepdims when an older numpy version is used, and could silently give wrong outputs that have the right shape. This would be bad. >>> import numpy as np
>>> import scipy.linalg
>>> a = np.arange(9, dtype=float).reshape((3, 3))
>>> b0 = np.linalg.norm(a, axis=0)
>>> b1 = scipy.linalg.norm(a)
>>> a*b0
array([[ 0. , 8.1240384 , 19.28730152],
[ 20.1246118 , 32.49615362, 48.2182538 ],
[ 40.24922359, 56.86826883, 77.14920609]])
>>> a*b1
array([[ 0. , 14.28285686, 28.56571371],
[ 42.84857057, 57.13142743, 71.41428429],
[ 85.69714114, 99.979998 , 114.26285486]]) How about raising an exception somewhere in this case? Or maybe it would be worth considering more extreme options? One is to have new scipy versions require a much more recent numpy version than 1.6. Another is to deprecate scipy linalg norm and replace it with numpy linalg norm. Of course both of these options have counter-arguments. |
I've tried something like that in #3610 but I remember it requiring too much ufunc hacking at the time. Maybe the situation has changed now that support for 1.5 has been dropped? |
My understanding is that if an axis argument is passed on an old version, numpy's norm function will throw an error because it received an unexpected keyword argument. The only distinction this function has over numpy's version is that it handles floating point overflow better. The BLAS version is actually marginally slower than the one in The ideal would probably be to rewrite this as a gufunc that calls the BLAS norm and then document why someone would want to call the scipy version rather than the numpy version. I haven't figured out the gufunc api yet though. This PR was just meant to fix the disparity between the function signatures. |
AFAIK, making a gufunc like this would also require numpy version 1.8, so it'll have to wait for a while too. |
Oops, I guess you are right. I think this PR would be OK to merge... |
The behavior is documented clearly in the docstring, and the message in the exception about "unexpected keyword" should be clear enough as well. So I agree with @argriffing that this PR is OK to merge. One test per keyword (can be copied from numpy I'd expect) marked to run only if the numpy version is high enough would be good to add. |
needs a rebase as well |
46cfbc2
to
bed43c2
Compare
…orm. Forward these arguments, if they are specified, to numpy.linalg.norm.
bed43c2
to
a8416dd
Compare
I've rebased this one, added tests, and made the logic a bit clearer. Now, whenever the axis and keepdims arguments are specified at all, everything is forwarded to It'd be nice if we could get this into the next release, though it's not at all a blocker. |
This looks OK to me. In an ideal world we do not depend on what is the underlying numpy version, but in this world this PR is OK I think, so +1 from me. |
ENH: Allow axis and keepdims arguments to be passed to scipy.linalg.norm.
No objections and LGTM, so in it goes. Thanks @insertinterestingnamehere! |
Thanks for reviewing! |
This allows the
axis
andkeepdims
keywords inscipy.linalg.norm
to be passed tonumpy.linalg.norm
whenever they take non-default values. These keywords may be rejected by numpy for older versions, but this avoids passing them through at all when the default values are used, so there is no lost functionality there. As to my knowledge, theaxis
keyword was added in numpy 1.7 and thekeepdims
keyword was added in 1.10. It would be really nice if they could be passed throughscipy.linalg.norm
though, for those of us with more recent versions.Most of the changes to the docstring are copied from the docstring for
numpy.linalg.norm
. I also added a note that the axis and keepdims keywords are only supported when using versions of numpy that support them.