I'd argue that isscalar is generally broken, and np.ndim(x) == 0 is normally the correct test

@eric-wieser - since it is used in the wild, we still might want to ensure it works as advertised. Possibly, we can add np.ndim(x) == 0 as a final stanza in the if statement?

@jdemeyer - I think the PR as stands is useful already - but could you add a test case?
Also, in the example, I think one should use an actual Fraction rather than one with no arguments.

I guess my point is that I would consider it to be a bug in scipy that isscalar is being used (or at least, could construct a failing tests case due to its use).

@eric-wieser - but where do we document this? Doesn't it make more sense that isscalar works as advertised? (Or, more specifically, that it solves the problem scipy has - I'm not 100% sure I know what that is, though!)

Also, more practically, this PR would seem to be an improvement no matter what...

@jdemeyer - I think the PR as stands is useful already - but could you add a test case?

What do you mean with a test case? Isn't the Fraction() test sufficient?

...or do you mean that I should implement a new class based on numbers.Number for testing?

Tests changed as requested.

@jdemeyer - yes, I meant a test with Fraction, but written in the corresponding test file - core/tests/test_numeric.py. I should note that this is sufficiently old that it doesn't actually test isscalar at all... So, it would need a new class; I think it is fine to just add the same tests as you currently have in the docstring; no need to have a tiny PR lead to a major test writing excercise!

p.s. Sadly, tests in the docs are actually not run in numpy...

p.s. Sadly, tests in the docs are actually not run in numpy...

OK, that explains your earlier comment.

@jdemeyer You should do your work in a branch rather than master.

You should do your work in a branch rather than master.

You are certainly right in principle, but for a one-off pull request like this, that doesn't matter.

Are the tests OK now?

Anything else preventing this to be merged?

I have a feeling this might change the behaviour of np.linspace for the worse, since there the use of isscalar is actually "is this a builtin scalar that behaves predictably, and won't lose information by casting".

I'd be happier if we could remove all internal uses of isscalar before merging this.

This seems reasonable to me. @mhvk I don't think this causes a problem with linspace. Is it the case that numbers with units are not considered Numbers?

@charris - our Quantity still would never cause a True, so I don't expect any problems from that side. Overall, as said before, I think this is an improvement, and I think we should have it even if eventually we might like to deprecate isscalar.

My question here was whether np.linspace ends up behaving worse for the sage types this set out to fix

My primary objection here is that if we're going to change behavior of np.isscalar, we may as well do it just once, and change the implementation to

def isscalar(x):
    return np.ndim(x) == 0

or perhaps with less breakage,

def isscalar(x):
    # still refuses to consider 0d arrays scalars
    return not isinstance(x, np.ndarray) and np.asarray(x).ndim == 0

There are other bugs (#9898) that this would fix at the same time

Your simplest example would definitely break things for Quantity - the second might well be OK, but would probably be quite a bit slower.

Maybe more relevantly, I don't think this has to hold up this particular PR: it just allows for classes that have specifically stated they are number-like by registering as such -- which I think is hard to argue against.

Anyway, my suggestion would be (1) merge this PR; (2) open new issue that we should look carefully in our own code base what isscalar is actually used for (changing to ndim == 0 wherever appropriate); (3) afterwards, decide whether we need isscalar at all, how it should behave, and deprecate if need be.

look carefully in our own code base what isscalar is actually used for

The answer is:

• That case in np.linspace
• np.piecewise, which is probably easy to fix
• np.histogramdd, which might be a little more work
• A poorly-behaved np.ma.apply_along_axis (MAINT: make np.ma.apply_along_axis consistent with np.apply_along_axis #8511)
• Code that we're only keeping around for compatibility anyway (best to leave?)
  • What is probably just an optimization in operator overloads in np.poly1d
  • Broken stuff in defmatrix that we probably can't change now anyway:

    >>> a[:,0]
    matrix([[ 1.],
            [ 0.]])
    >>> a[:,np.array(0)]
    matrix([[ 1.,  0.]])
    

well, it sounds like step (2) would be relatively easy! (In the meantime, this PR still fixes a real bug... even if it is in scipy.)

If @kiwifb can confirm that linspace still does the right thing on sage integers after this patch, then I suppose I'm +0 on this

Haven't heard back from @kiwifb . I am going to put this in, we can fix it later if things go wrong.

Thanks @jdemeyer .

Sorry, was a bit busy. We have been using this patch (@jdemeyer wrote it for sage after all) for the last month or so and we haven't seen side effects.

So np.linspace(sage_int, sage_int) returns the same type as before? I actually meant to tag @jdemeyer there, you just made it sound like this was your patch in that comment!

I don't see a difference before and after.