MAINT: Use a StructSequence in place of the typeinfo tuples #10154
Conversation
34b13b4 to
ad74ecc
Compare
This makes the contents of `typeinfo` look liked namedtuples
ad74ecc to
4b613a2
Compare
| types.append(Type(name)) | ||
| return types | ||
|
|
||
| def larger_types(self): | ||
| bits = typeinfo[self.NAME][3] |
There was a problem hiding this comment.
Hey, the refactor was useful! Seems this has exposed a bug in what was already here... ([3] is .alignment under the new spelling)
|
This looks very nice! Definitely substantially clearer. |
| self.dtypechar = typeinfo[self.NAME][0] | ||
| assert_equal(self.type_num, info.num) | ||
| self.dtype = info.type | ||
| self.elsize = info.bits / 8 |
There was a problem hiding this comment.
I know it was here already, and it is only a test, so perhaps OK, but it still seems strange not to use //.
There was a problem hiding this comment.
Actually, I'd argue that all of these are wrong, and should be ceil(bits / 8) or must_be_integral(bits / 8). Would prefer to leave that out of scope, either way
numpy/core/numerictypes.py
Outdated
| for info, charname, intname, Intname in [ | ||
| (i_info,'i%d' % (bits//8,), 'int%d' % bits, 'Int%d' % bits), | ||
| (u_info,'u%d' % (bits//8,), 'uint%d' % bits, 'UInt%d' % bits) | ||
| ]: |
There was a problem hiding this comment.
Might put this at the end of the previous line and indent both lines another 4 spaces.
There was a problem hiding this comment.
Also reasonable - feel free to edit in the web editor, because I'm not going to get around to checking out this branch today.
Out of curiousity, does PEP8 have anything to say here?
There was a problem hiding this comment.
It doesn't mention the for case explicitly, and the if version leaves it pretty open. I'm just thinking C here. It would also be reasonable to just indent the ]:.
numpy/core/src/multiarray/typeinfo.c
Outdated
| }; | ||
|
|
||
| static PyStructSequence_Desc typeinfo_desc = { | ||
| /* name */ "numpy.core.multiarray.typeinfo", |
There was a problem hiding this comment.
The comment would normally go after the entry.
There was a problem hiding this comment.
I think it would also look better, especially as it would be expected.
There was a problem hiding this comment.
Column alignment is easier to maintain this way, but I think you're right that consistency wins here, and cpython uses what you suggest internally
|
Thanks Eric. |
Part of trying to dissect the problem with #10151.
Replaces the tuples in
np.core.multiarray.typeinfowith struct sequences (which in Py3 only, subclasstuple).This makes this all a little more readable.
Also adds a bunch of error checking while I'm here.