MAINT: refactor: arraydescr_new now does not call PyArray_DescrNew by mattip · Pull Request #12430 · numpy/numpy

mattip · 2018-11-20T22:06:31Z

I started making dtype classes so that the type(np.dtype('int32')) is no longer numpy.dtype rather a new numpy.int_type type that inherits from numpy.dtype, the first step in the plan to refactor dtypes. Rather than a single large PR, I will try to do this in a set of smaller PRs, with an "opt-out" #ifdef ... #else ... #endif around the code.

This PR is the first in the series - a refactoring of the dependency between arraydescr_new and PyArray_DescrNew so that the latter now calls the former instead of the other way around. ~~I also refactored the function to create datetime64 metadata and moved it to a different place in the file.~~

Benchmarking seemed to show no real difference in performance, although there is alot of variance in the bench_core.CountNonzero.time_count_nonzero tests

numpy/core/src/multiarray/arraytypes.c.src

eric-wieser · 2018-11-20T22:28:54Z

Datetime cleanup looks good too, but completely unrelated to your PR title - can you split that off into a separate PR?

eric-wieser · 2018-11-22T13:23:26Z

numpy/core/src/multiarray/descriptor.c

Are these bug fixes, or could this previously never return NULL?

As PyArray_DESCR_REPLACE calls PyArray_DescrNew which has various failure modes, it is a bugfix

refactored these into PR #12546

eric-wieser · 2018-11-24T01:09:19Z

numpy/core/src/multiarray/descriptor.c

+                                  /* dtype */ base,
+                                  /* align */ Py_False,
+                                  /* copy  */ Py_True);
+    PyObject *ret = PyObject_CallObject((PyObject*)Py_TYPE(base), tuple);


PyObject_CallFunctionObjArgs((PyObject *)Py_TYPE(base), base, Py_False, Py_True) would be shorter here - or am I missing something?

eric-wieser · 2018-11-24T01:13:46Z

numpy/core/src/multiarray/descriptor.c

        newdescr->subarray = PyArray_malloc(sizeof(PyArray_ArrayDescr));
        if (newdescr->subarray == NULL) {
            Py_DECREF(newdescr);
-            return (PyArray_Descr *)PyErr_NoMemory();


This PyErr_NoMemory() needs to remain

revert removal

eric-wieser · 2018-11-24T01:20:08Z

numpy/core/src/multiarray/descriptor.c


+/* Initialize the PyArray_Descr fields */
+static int
+_initialize_descr(PyArray_Descr *newdescr, PyArray_Descr *base)


I'd prefer this function to be:

static PyObject *descr_copy(PyArray_Descr *base) { newdescr = PyObject_New(PyArray_Descr, Py_TYPE(base)); // as below return newdescr }

or if for some reason, we want to be able to change the type of the copy.

// classmethod static PyObject *descr_from_descr(PyTypeObject *cls, PyArray_Descr *base) { newdescr = PyObject_New(PyArray_Descr, cls); // as below return newdescr }

Is there a erason that's a bad idea?

Adopting the second option, so we can, in the future, subclass dtype

eric-wieser · 2018-11-25T16:20:33Z

I started making dtype classes so that the type(np.dtype('int32')) is no longer numpy.dtype rather a new numpy.int_type type that inherits from numpy.dtype,

I'm not sure this is where we want to end up. The model that I think we should be aiming for is:

# dtype is a metaclass
>>> issubclass(np.dtype, type)
True

# dtypes instances are still just that - dtype instances
>>> i = np.dtype('int32')
>>> type(i)
np.dtype

# but they're also now real python types
>>> isinstance(i, type)
True

# meaning they can be used as constructors
>>> type(i(1.0)) is i
True

# and eventually, probably become one with the scalar types
>>> i is np.int32
True

I think that matches up with your dtype task list too.

np.int_type shouldn't be a subclass of dtype in my mind - it should be an instance of dtype.

With that in mind, I'm not sure this patch really makes any sense - I can't think of a good reason to subclass dtype right now.

Sorry for not making this remark earlier in the review.

Would be great if you could split off the bug fixes here so that those can make 1.16

mattip · 2018-11-25T17:01:26Z

With that in mind, I'm not sure this patch really makes any sense - I can't think of a good reason to subclass dtype right now.

This is one step toward enabling proper subclassing of dtypes, needed for projects like units and categorical dtypes. The first step is allowing creation of other dtype types (not objects), while keeping the array.dtype attribute an object. The final goal is indeed to make array.dtype a type object. Even better would be that making an instance of the dtype would be a scalar.

While this PR is only a minor refactoring, it fixes the order of arraydecr_new and the API function PyArray_DescrNew. The internal tp_new should not be calling out to an API function, so the PR is valuable on its own.

eric-wieser · 2018-11-25T17:45:30Z

This is one step toward enabling proper subclassing of dtypes

As I read the task document, the intent was to subclass dtype instance s, not dtype itself.

I cannot think of a single use case right now for subclasses of dtype itself.

The internal tp_new should not be calling out to an API function, so the PR is valuable on its own.

It's not clear that this is obviously true. The python analogy to what we currently have would be a __new__ implemented as:

def __new__(cls, arg):
    if isinstance(arg, dtype):
        return dtype.from_dtype(arg)
    elif isinstance(arg, str):
        return dtype.from_descr(arg)

@staticmethod
def from_dtype(arg):
    # This is PyArray_DescrNew, written as python
    self = object.__new__(dtype)
    self.elsize = arg.elsize  # etc
    return self

Where dtype.__new__ is just a dispatcher that picks between a bunch of API functions based on the type of the argument.

The analogy to your patch is a patch that:

Removes all callers of dtype.from_dtype, and make them go through type dispatching even though they have no need to.
Changes the staticmethod to a classmethod - it's not clear to me that this buys anything, but I suppose it's not harmful.

mattip · 2018-11-25T18:44:46Z

Is subclassing an instance a common python idiom? I thought subclasses must inherit from a type, not an instance. Are there restrictions on methods called on a class that inherits from an instance?

eric-wieser · 2018-11-25T18:49:47Z

Is subclassing an instance a common python idiom? I thought subclasses must inherit from a type

No, you're correct. My point is that subclasses of dtype doesn't seem like a stepping stone to where we want to end up. I think the first step should be to make dtype a subclass of type, not the last step.

mattip · 2018-11-25T18:59:07Z

well, it making dtype a subclass of type is the next step, and will not be the last step. I need the change to use subtype in PyObject_New. I guess I could redo this PR to do that with a more minimal change, but it seems unnatural to me that arraydescr_new, which is type->tp_new is calling out to an API function to actually allocate, not the other way around.

Edit: clarify "it"

eric-wieser · 2018-11-25T19:17:03Z

I need the change to use subtype in PyObject_New.

I understand this - it's that next step specifically that I think is in the wrong direction, and something we'll end up undoing. Once dtype becomes a metatype, then we'll end up reverting back to using PyObject_New called on dtype, not subclasses.

Could you maybe update your document with the proposed steps here? Perhaps I'm not seeing the bigger picture.

but it seems unnatural to me that arraydescr_new, which is type->tp_new is calling out to an API function to actually allocate, not the other way around.

It's worth looking at the immutable types in CPython for precedence here. PyFrozenSet_New does not call frozenset->tp_new at all. Nor is PyLongObject->tp_new called by any of the C api constructors.

I think it's better to view __new__ as the python-only overloaded entry point to the construction, and not something the C api should go through. Extracting a common helper function for both to go through is fine (the descr_from_descr function of this patch), but since CPython doesn't do it either, I don't think it makes sense for the C api to round trip through the python api.

mattip · 2018-11-28T19:09:50Z

Could you maybe update your document with the proposed steps here? Perhaps I'm not seeing the bigger picture.

I am kind of working it out as I go. I think I know what I want, but I am not sure how to get there. Any help or critique is helpful, thanks for the insights.

mattip · 2018-12-23T09:14:42Z

Closing this in favor of other approach

mattip added 03 - Maintenance component: numpy.dtype labels Nov 20, 2018

eric-wieser reviewed Nov 20, 2018

View reviewed changes

numpy/core/src/multiarray/arraytypes.c.src Outdated Show resolved Hide resolved

eric-wieser reviewed Nov 20, 2018

View reviewed changes

numpy/core/src/multiarray/arraytypes.c.src Outdated Show resolved Hide resolved

mattip mentioned this pull request Nov 22, 2018

MAINT: refactor datetime.c_metadata creation #12437

Merged

mattip force-pushed the dtype-refactor branch from 3fc5f48 to 585d4fe Compare November 22, 2018 13:02

mattip changed the title ~~MAINT: refactor dtype so that arraydescr_new does not call PyArray_De…~~ MAINT: refactor: arraydescr_new now does not call PyArray_DescrNew Nov 22, 2018

eric-wieser reviewed Nov 22, 2018

View reviewed changes

MAINT: refactor: arraydescr_new now does not call PyArray_DescrNew

1993561

mattip force-pushed the dtype-refactor branch from 585d4fe to 1993561 Compare November 22, 2018 17:47

eric-wieser reviewed Nov 24, 2018

View reviewed changes

MAINT: fixes from review

56c94aa

mattip mentioned this pull request Nov 28, 2018

WIP: ENH: make dtypes a PyTypeObject #12462

Closed

mattip mentioned this pull request Dec 14, 2018

BUG: check for errors after PyArray_DESCR_REPLACE #12546

Merged

MAINT: refactor out into two PRs

dc7c78d

mattip mentioned this pull request Dec 18, 2018

WIP: refactor dtype to be a type subclass #12585

Closed

charris mentioned this pull request Dec 18, 2018

BUG: check for errors after PyArray_DESCR_REPLACE #12588

Merged

mattip closed this Dec 23, 2018

mattip deleted the dtype-refactor branch June 8, 2020 06:57

Uh oh!

Comments

Conversation

mattip commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eric-wieser commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 25, 2018

Uh oh!

eric-wieser commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 25, 2018

Uh oh!

eric-wieser commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Nov 25, 2018

Uh oh!

mattip commented Nov 28, 2018

Uh oh!

mattip commented Dec 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mattip commented Nov 20, 2018 •

edited

Loading

eric-wieser commented Nov 20, 2018 •

edited

Loading

eric-wieser Nov 24, 2018 •

edited

Loading

eric-wieser commented Nov 25, 2018 •

edited

Loading

eric-wieser commented Nov 25, 2018 •

edited

Loading

eric-wieser commented Nov 25, 2018 •

edited

Loading

mattip commented Nov 25, 2018 •

edited

Loading