ENH: Reduce compute time for tobytes in non-contiguos paths
#30170
Conversation
|
Quick comment: benchmarks belong in benchmarks/benchmarks, not in tests |
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks, this looks good, but I think we can simplify it a fair bit.
numpy/_core/src/multiarray/convert.c
Outdated
|
|
||
| /* Writable Buffer */ | ||
| char* dest = PyBytes_AS_STRING(ret); | ||
| memset(dest, 0, numbytes); |
There was a problem hiding this comment.
You don't need this if you have the NEEDS_INIT check above.
numpy/_core/src/multiarray/convert.c
Outdated
| for (int i = 0; i < PyArray_NDIM(self); i++) { | ||
| strides[i] = stride; | ||
| stride *= PyArray_DIM(self, i); | ||
| } |
There was a problem hiding this comment.
You should't need any of this, PyArray_NewFromDescr should fill in the strides for you.
numpy/_core/src/multiarray/convert.c
Outdated
| if (ret == NULL) { | ||
| Py_DECREF(it); | ||
|
|
||
| PyArray_CLEARFLAGS(dest_array, NPY_ARRAY_OWNDATA); |
benchmarks/benchmarks/bench_core.py
Outdated
| self.noncontig.tobytes() | ||
|
|
||
| def time_tobytes_contiguous(self): | ||
| np.ascontiguousarray(self.noncontig).tobytes() |
There was a problem hiding this comment.
I don't think we should time this. You could use .T just directly in the non-contiguous version also (since the .T will be irrelevantly fast here).
numpy/_core/tests/test_multiarray.py
Outdated
| pytest.raises(ValueError, a.getfield, 'uint64', 0) | ||
|
|
||
| @pytest.mark.parametrize("shape", [(3, 224, 224), (8, 512, 512)]) | ||
| def test_tobytes_noncontiguous_not_slower_than_copy(shape): |
There was a problem hiding this comment.
Can you rename this to just _no_copy_fastpath or so and see if there are other ravel tests where this might fit well. (e.g. we often have a TestRavel class).
There was a problem hiding this comment.
I think TestRegression is the place where many of the ravel and tobytes methods are tested.
There was a problem hiding this comment.
test_multiarray has a few test_ravel* functions, below those would be nicest I think.
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
LGTM, moving the tests would be nice, tiny typos if you iterate anyway.
numpy/_core/src/multiarray/convert.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| /* Avoid Ravel where possible for lesser copies. */ |
There was a problem hiding this comment.
| /* Avoid Ravel where possible for lesser copies. */ | |
| /* Avoid Ravel where possible for fewer copies. */ |
numpy/_core/src/multiarray/convert.c
Outdated
| if (!PyDataType_REFCHK(PyArray_DESCR(self)) && | ||
| ((PyArray_DESCR(self)->flags & NPY_NEEDS_INIT) == 0)) { | ||
|
|
||
| /* Allocate final Byte Object */ |
There was a problem hiding this comment.
| /* Allocate final Byte Object */ | |
| /* Allocate final Bytes Object */ |
|
Thanks for the quick review @seberg |
|
Thanks, I think this amount of added complexity is a simple enough to warrant adding it. |
…30170) * Add optimal copy path for non-contiguos arrays in ToString C Impl. * Add tests for obytes new path * Add imports * fix comments * fix memory issues and add benchmarks * run ruff --fix * simplify function * minor fix * minor typos and move tests
…30170) * Add optimal copy path for non-contiguos arrays in ToString C Impl. * Add tests for obytes new path * Add imports * fix comments * fix memory issues and add benchmarks * run ruff --fix * simplify function * minor fix * minor typos and move tests
3 participants
Fixes #30048
This PR adopts better copying strategies than the previous element by element copying for non-contiguos paths.