MAINT/BUG: Fixups to scalar base classes #15393
Conversation
| } \ | ||
| /* now do default conversion */ \ | ||
| } | ||
| assert (type->tp_bases && (PyTuple_GET_SIZE(type->tp_bases) == 2)); \ |
There was a problem hiding this comment.
This assertion is failing for subclasses of np.float64. The fact that it is suggests that this code isn't doing what it's supposed to be doing anyway though...
There was a problem hiding this comment.
This is patently scary... Is there a reason it is not hardcoding sup directly?
There was a problem hiding this comment.
Agreed, very scary - fixed in the follow-up commit.
There was a problem hiding this comment.
Can +we remove the num completely, as far as I can tell, it basically maps to a known instance type?
There was a problem hiding this comment.
It does indeed map to a known type, but getting hold of that type through the template mechanism is harder than grabbing it at runtime. Essentially you're choosing between:
class A(B, C):
def __new__(cls, ...):
C.__new__(...)and
class A(B, C):
def __new__(cls, ...):
A.__bases__[1].__new__(...).
Both are completely correct, unlike what was there before which was outright stupid:
class A(B, C):
def __new__(cls, ...):
cls.__bases__[1].__new__(...)There was a problem hiding this comment.
Yes, the first seemed nicer to me? although not sure if we have the PyFloatType name available easily. Could also think about __bases__[-1]. in any case, not sure it would make things better or worse, so OK with just leaving it and not doing the last thing is a nice fix.
There was a problem hiding this comment.
We can't use -1 because for unicode_ and bytes_ it's __bases__[0], but for float_ it's __bases__[1] - for bizarre reasons the order is deliberately swapped.
There was a problem hiding this comment.
I suspect I will need to make further cleanups here to address #15385 anyway, in future PRs.
10bf51c to
c41fde5
Compare
This seems to be left from when `np.int_` subclassed `int`, which it no longer does. Change the `if` to an `assert` so that we can catch this type of change in future.
c41fde5 to
30f534a
Compare
|
Rebased locally with the above changes |
The previously code was equivalent to something like
```python
class SomeScalar(base1, base2):
def __new__(cls, *args):
return cls.__bases__[1].__new__(*args)
```
This is nonsense and can easily cause recursion, for instance if `cls` is a superclass of SomeScalar.
The correct way to spell this is to pin the base class call statically.
```python
class SomeScalar(base1, base2):
def __new__(cls, *args):
return SomeScalar.__bases__[1].__new__(*args)
```
Fixes numpy#15395
30f534a to
c0ca814
Compare
|
Thanks @eric-wieser, @seberg |
This seems to be left from when
np.int_subclassedint, which it no longer does.Change the
ifto anassertso that we can catch this type of change in future.