MAINT: Fix casting signatures to align with NEP 43 signature by seberg · Pull Request #18398 · numpy/numpy

seberg · 2021-02-11T23:41:09Z

This changes all casting loops to get passed in a new context
which holds some data such as the descriptors (and methods).
For the most part, this is unnecessary information, but it
allows us to pass additional information to ufuncs while also
aligning ufunc and casting signatures (and definition).

To achieve this, we have to fill and store this context
everywhere, which leads to some rework.
That means that this PR changes some of the chained loops
to use fewer specialized loops.

There should be no typical speed effect for larger arrays
aside possibly a small one for rarer cases such as byteswapped
or unaligned arrays (although we currently consider strings
unaligned much too often).

However, there will be a small increase in overheads, timing
it, it appears to be <10% in the most extreme cases.
While not neglible there are bigger optimizations possible
(even moving the buffer-info data to the array struct was
a bigger speedup in most cases!).

I think this should be ready to merge, but I am happy to rename things for example (I have not thought about names too much). Personally, I am unconcerned about performance right now, we have much bigger overheads to get rid of (even the alignment check is slower), and enough potential for optimizing those overheads away and this will make that easier rather than harder, I think.

I will probably still add some tests, especially some of those "copy auxdata" functions probably never had a test and now that they are modified codecov will realize that.

Astropy tests run identically with master and this for me.
pandas test runs identically (2 less failures, I will attribute it to some flaky tests using network or running the tests in parallel)

Overall the changes are pretty simple, its mostly easy to get things wrong like mixing up source and destination strides in fields.

TODOs (but doesn't need to be strictly part here):

string/unicode <-> datetime/timedelta seem a bit undertested, e.g. with respect to byte swapping (an error was only found by some recfunction tests which happened to use < meaning it was byte swapping on a big endian machine).

This changes all casting loops to get passed in a new `context` which holds some data such as the descriptors (and methods). For the most part, this is unnecessary information, but it allows us to pass additional information to ufuncs while also aligning ufunc and casting signatures (and definition). To achieve this, we have to fill and store this context everywhere, which leads to some rework. That means that this PR changes some of the chained loops to use fewer specialized loops. There should be no typical speed effect for larger arrays aside possibly a small one for rarer cases such as byteswapped or unaligned arrays (although we currently consider strings unaligned much too often). However, there will be a small increase in overheads, timing it, it appears to be <10% in the most extreme cases. While not neglible there are bigger optimizations possible (even moving the buffer-info data to the array struct was a bigger speedup in most cases!). Below are individual (squashed) commit while working on this, since the change had to be mostly complete to compile/test its fairly long (leaving it just for fun) Individual commit ----------------- Further changes Fixup the main multi-step cast functions Restructure field transfers (partially?) continuation WIP: This is tricky, need to redo some stuff (this is the start of that redo) WIP: more small changes And more work... Clean out/simplify legacy multi-step casts (mainly datetime) Fixup strided one to n field transfer (and decref partially) Fixup n-to-n cast for new methods Fix subarray casts for the new calling convention Finish fixing up the structured transfers and decref start with cleaning up masked casts BUG: Fixup struct access typo Fixup any-to-object Finish all/most of masked casting, this brings me to debugging... Fixup object copy and move functions Code now compiles without errors (of course segfaults immediately :)) Finish fixup of array_assign_* Pass pointer to cast_info_xfree (to be able to NULL func and error check) Also silence a few warnings Bug: Make sure datetime casts have copy/copyswap only path This is necessary to ensure the recursive calls don't recurse forever :). Small fixes, mostly make sure we use PyMem_* for all allocations/frees BUG: Fix memset in multistep cast Seems that code was little enough tested, that most test runs would simply not notice the issue (i.e. requires debug mode usually probably). Now it is used a lot and things came down hard. Bug: Fix stride order typo affecting strided masked indexing BUG: Finish npy-iter copy fixup (apparently it was not...) BUG: Fix scalar assignment mixup Fix bug in field bookkeeping and rename decsrcref to decref Its called "decref" everywhere, the "src" is just confusing especially since now we don't have the `func(dst, src)` calling convention anymore. Final fixes and cleanup of comments e.g. removed some comments alluring to a future that is unlikely to ever come... DOC: Change casting error message to be closer to other messages Fixup rebase: masked transfer function initialize got lost Fixup: remove print added for debugging into recfunctions

These is probably fairly common, so optimize the lookup in this extremely simple case. More optimizations should probably done eventually, e.g. we could add a fast-list for all builtin ones, or have an "intern" mechanism so that casts for any interned dtype will be stored in a list, with the assumption that an interned dtype can e.g. never be deleted and that there should not be too many of them.

mattip · 2021-02-12T09:16:20Z

         * This should potentially become a weak mapping in the future.
         */
+        PyObject *within_dtype_castingimpl;
        PyObject *castingimpls;


The comment remains unchanged.

Since these are C-level objects, would we gain anything by making them a pointer-to-function rather than a python PyObject* ? If they need to be exposed to python, they could be wrapped before being returned. For user-defined dtypes, the function could use the python C-API to call back out to python,

These are ArrayMethods. Somehow, I don't have a clear mental model for making something that may be defined in Python a bare C-struct (or the advantage over it just beeing it a PyObject *). If we may want to be able to delete a dtype (and with it these casts), it seems to me that PyObject * is more natural than a plain struct.

I somewhat understand the desire to keep out Python where possible, but not sure here. Maybe I am just missing the right design patterns for this?

I guess we can revisit it in the future.

mattip · 2021-02-12T14:57:45Z

-                            PyArray_StridedUnaryOp **out_stransfer,
-                            NpyAuxData **out_transferdata,
+                            NPY_cast_info *cast_info,
                            int *out_needs_api);


You can remove out_stransfer and out_transferdata from the list of paramters in the comment above

mattip · 2021-02-12T15:02:27Z

Seems reasonable. This is quite a lot of lines to review, so I will let it sit a bit before final approval in case someone else wants to look it over.

seberg · 2021-02-12T15:35:06Z

Yes, please let it sit for a few days at least, running valgrind and maybe a leak checker later also, seems to be at least one negligible leaks in an error path (until now).

About having these things be PyObjects rather than a lower struct. As long as you do not think it will maneuver us into a dead end, I am happy to ignore the issue, to be honest. But if it might, I am worried. Aside from that, though, it will probably be easier to put this in and refactor from there, the hurdle is mainly when we make it public API.

This could maybe be done a bit nicer, for now a small (local) helper seemed convenient. This is only needed here.

There were some paths that manipulated the unused data. This was fixed in the big refactor (or earlier), I expect there were some fast paths for equal sized dtypes that kicked in before

rossbar

I'm not capable of seeing the forest through the trees, but I tried to keep an eye out for things like argument remappings always having consistent order (e.g. args -> {src, dest} consistently).

rossbar · 2021-02-17T06:28:29Z

 *      The auxiliary data for the transfer function is placed here.
 *      When finished with the transfer function, the caller must call
 *      NPY_AUXDATA_FREE on this data.
+ * context:


The comment seems out-of-sync with the signature - should this describe cast_info instead?

The parameter descriptions here are not aligned with the changes in the signature

rossbar · 2021-02-17T07:21:55Z


-    auxdata->base.free = &_multistep_cast_auxdata_free;
-    auxdata->base.clone = &_multistep_cast_auxdata_clone;
+    /* Initialize func's to NULL to signal no-cleanup in case of an error. */


Suggested change

/* Initialize func's to NULL to signal no-cleanup in case of an error. */

/* Initialize funcs to NULL to signal no-cleanup in case of an error. */

rossbar · 2021-02-17T07:30:45Z

+ * casts (e.g. structured dtypes).
+ *
+ * This struct provides a place to store all necessary information as
+ * compact as possible.  It must be used with the inlie functions below


Suggested change

* compact as possible. It must be used with the inlie functions below

* compact as possible. It must be used with the inline functions below

As noted by Matti and Ross, these comments were incorrect. Co-authored-by: Matti Picus <[email protected]> Co-authored-by: Ross Barnowski <[email protected]>

Not the prettiest tests, but should ensure that casts involving byteorder changes within datetime/timedelta and between datetimes or timedeltas and strings are handled correctly.

seberg · 2021-02-17T19:20:01Z

OK, added a slightly ugly test for the datetime/timedelta (together with string mostly) byteswapping-casts. That should remove the main hole in the test coverage.

My valgrind tests are finally completing, and I have not found any additional issues to do with these changes. So I am happy for now.

seberg · 2021-03-16T16:03:17Z

I would like to push this over the finish line soon. First, to get a bit of testing and second so I can get just fully get rid of that "use new casting implementation" flag again in the next PR (that might have a slightly detrimental effect on ufunc overheads, but its a chicken-egg problem).

I realize that this makes a big step towards the NEP 43-like organization, but I think the only thing truly baked in, is the fact that we pass in an additional context (whatever is actually stored in there) to ufuncs in the future. Everything else seems pretty flexible at this point.

mattip · 2021-03-17T12:37:52Z

+            PyErr_Format(PyExc_RuntimeError,
+                    "A cast was already added for %S -> %S. (method: %s)",
+                    meth->dtypes[0], meth->dtypes[1], meth->method->name);
+            return -1;


If this could be hit by a user-defined type (as I think it could), it would be good to test for

It can't be hit by a current user defined dtype. I could currently only hit it by adding a cast twice to a NumPy builtin DType.

Please add a comment then that it is an internal error, and will never be hit in practice

mattip · 2021-03-17T12:39:51Z


+    PyArray_Descr *src_wrapped_dtype = ensure_dtype_nbo(descrs[0]);
+    PyArray_Descr *dst_wrapped_dtype = ensure_dtype_nbo(descrs[1]);
+


It seems this code path is not hit?

mattip · 2021-03-17T12:45:41Z

-                            count, src_itemsize, data);
+                    return stransfer(&cast_info->context,
+                            args, &count, strides, mask,
+                            mask_stride, cast_info->auxdata);


this seems like a false negative, surely we are hitting this code path?

Hmmm, probably not a false positive... These are masked buffered casts that are only used in NpyIter so they are pretty darn specific and little used. (And this branch is probably specific to large arrays, have to think a bit how to test it right.)

I added/expanded a test... Its not pretty, and confusing though :/.

It seemst he unicode didn't actually help coverage, but here we go.

This is utterly confusing, but adds coverage to another uncovered pass. I honestly do not really understand right now, why "writemasked" and "buffered" combination can bypass the buffering here. I guess there some special logic about that in the `where=` ufunc code that uses the masked versions here?

mattip · 2021-03-18T04:50:40Z

The linter is not pleased:

/numpy/core/tests/test_datetime.py:712:80: E501 line too long (80 > 79 characters)
./numpy/core/tests/test_datetime.py:735:80: E501 line too long (85 > 79 characters)

and there is a failing test on no_relaxed_strides

    def test_unicode_byteswapped_cast(self, order1, order2):
        # Very specific tests (not using the castingimpl directly)
        # that tests unicode bytedwaps including for unaligned array data.
        dtype1 = np.dtype(f"{order1}U30")
        dtype2 = np.dtype(f"{order2}U30")
>       data1 = np.empty(30 * 4 + 1, dtype=np.uint8)[None, 1:].view(dtype1)
E       ValueError: To change to a dtype of a different size, the array must be C-contiguous

…g error

mattip · 2021-03-18T19:10:28Z

Thanks @seberg

github-actions Bot added the 03 - Maintenance label Feb 11, 2021

seberg force-pushed the casting-loop-signature branch from 97bcb3f to 50dd7a0 Compare February 12, 2021 00:52

seberg added 2 commits February 11, 2021 22:16

BUG: string->datetime64 does not support byte swapping

2581703

seberg force-pushed the casting-loop-signature branch from 39e4a72 to 2581703 Compare February 12, 2021 04:45

mattip reviewed Feb 12, 2021

View reviewed changes

seberg mentioned this pull request Feb 13, 2021

BUG: Stuctured field dtype assignment can modify data not included in dtype #12686

Closed

seberg added 2 commits February 12, 2021 21:12

BUG: Fix cleanup on get_loop failure

1cce5b9

This could maybe be done a bit nicer, for now a small (local) helper seemed convenient. This is only needed here.

TST: test_sparse_field_assignment now succeeds now (was xfail)

1eff37a

There were some paths that manipulated the unused data. This was fixed in the big refactor (or earlier), I expect there were some fast paths for equal sized dtypes that kicked in before

rossbar reviewed Feb 17, 2021

View reviewed changes

DOC: Fix comments

4b034a9

As noted by Matti and Ross, these comments were incorrect. Co-authored-by: Matti Picus <[email protected]> Co-authored-by: Ross Barnowski <[email protected]>

seberg force-pushed the casting-loop-signature branch from cc4735d to 4b034a9 Compare February 17, 2021 18:23

TST: Add test for datetime and datetime->string byte order casts

1867278

Not the prettiest tests, but should ensure that casts involving byteorder changes within datetime/timedelta and between datetimes or timedeltas and strings are handled correctly.

Base automatically changed from master to main March 4, 2021 02:05

h-vetinari mentioned this pull request Mar 9, 2021

BUG: astype(object) downcasts for datetime-dtype #12550

Open

mattip reviewed Mar 17, 2021

View reviewed changes

Comment thread numpy/core/src/multiarray/lowlevel_strided_loops.c.src

mattip reviewed Mar 17, 2021

View reviewed changes

seberg added 2 commits March 17, 2021 19:03

TST: Add tests for datetime byteswaps and unicode byteswap casts

1258a03

It seemst he unicode didn't actually help coverage, but here we go.

MAINT: Add comment to unused func, silence linter and avoid non-conti…

b5de1ce

…g error

mattip approved these changes Mar 18, 2021

View reviewed changes

mattip merged commit bb7a31a into numpy:main Mar 18, 2021

seberg deleted the casting-loop-signature branch March 18, 2021 19:12

bonpyt mentioned this pull request Jan 20, 2025

BUG: Significant copy performance regression for structured arrays #28194

Closed

	/* Initialize func's to NULL to signal no-cleanup in case of an error. */
	/* Initialize funcs to NULL to signal no-cleanup in case of an error. */

	* compact as possible. It must be used with the inlie functions below
	* compact as possible. It must be used with the inline functions below


		PyArray_Descr *src_wrapped_dtype = ensure_dtype_nbo(descrs[0]);
		PyArray_Descr *dst_wrapped_dtype = ensure_dtype_nbo(descrs[1]);

Conversation

seberg commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip Feb 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Feb 12, 2021

Uh oh!

seberg commented Feb 12, 2021

Uh oh!

rossbar left a comment

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!

seberg commented Feb 17, 2021

Uh oh!

seberg commented Mar 16, 2021

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!

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!

mattip commented Mar 18, 2021

Uh oh!

mattip commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seberg commented Feb 11, 2021 •

edited

Loading

mattip Feb 12, 2021 •

edited

Loading