Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix order='K' behavior for tobytes #16328

Closed
wants to merge 1 commit into from

Conversation

anirudh2290
Copy link
Member

@anirudh2290 anirudh2290 commented May 21, 2020

Simplify PyArray_ToString to call PyArray_Ravel internally.
This addresses #16569 (see also: #16291 (comment))

Py_DECREF(ravel_out);

ret = PyBytes_FromStringAndSize(NULL, (Py_ssize_t) numbytes);
if (ret == NULL) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we need this here just as a micro opt or if there is a deeper reason ?

|| (PyArray_IS_F_CONTIGUOUS(self) && (order == NPY_FORTRANORDER))) {
ret = PyBytes_FromStringAndSize(PyArray_DATA(self), (Py_ssize_t) numbytes);

ravel_out = PyArray_Ravel(self, order);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would appreciate input on a better name here.

@seberg
Copy link
Member

seberg commented May 21, 2020

Sorry, I see I misjudged this a bit. The current algorithm uses an iterator while ravel would create a copy if this is not already contiguous. I think the safer way might be to:

  1. Fast-path when the contiguity is correct (this is the first if and is already correct).
  2. Calling ravel will work, but may create an additional copy unfortunately (I somewhat expected we do that anyway, but we don't...), its just one copy more though, maybe we should not care too much...

Avoiding the second copy, can be done by either using the new iterator:

/* no need to handle empty (always contiguous) */
flags = NPY_ITER_READONLY | NPY_ITER_GROWINNER | NPY_ITER_EXTERNAL_LOOP | NPY_ITER_DONT_NEGATE_STRIDES;
iter = NpyIter_New(op,,
                    order, NPY_NO_CASTING, NULL);

prepare_things;
do inner-loop {
    memcpy();
while (iternext())

although that is a bit annoying, but if you would like to get to know that iterator API maybe not a bad exercise.

@seberg
Copy link
Member

seberg commented May 21, 2020

Or maybe we just put the axis-reorganizing ravel does into an internal helper, so that we do not have to duplicate it.

return NULL;
}
NPY_BEGIN_THREADS_DEF;
needs_api = NpyIter_IterationNeedsAPI(iter);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was a bit confused about the needs api stuff and the macros for NPY_BEGIN_THREADS_DEF , NPY_BEGIN_THREADS_NDITER etc. My understanding is that we just want to hold the GIL and I wasn't sure if we need any of this stuff.

Base automatically changed from master to main March 4, 2021 02:04
Copy link
Contributor

@shubham11941140 shubham11941140 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will give a wrong result as LHS = 2 whereas the RHS = 3.
When the order of 'x' will be in K, then the tobytes function should hold.

@@ -4714,6 +4714,10 @@ def test_empty_files_text(self):
y = np.fromfile(self.filename, sep=" ")
assert_(y.size == 0, "Array not empty")

def test_tobytes_order(self):
x = np.array([[1, 2, 3], [4, 5, 6]], order='F')
assert_(x.tobytes('F') == x.tobytes('K'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will give a wrong result as LHS = 2 whereas the RHS = 3.
When the order of 'x' will be in K, then the tobytes function should hold.

@seberg
Copy link
Member

seberg commented Aug 22, 2021

The test looks correct to me. If it fails, the code needs fixing, not the test.

@charris
Copy link
Member

charris commented Jun 9, 2022

@anirudh2290 Needs a rebase. Do you want to pursue this?

@anirudh2290
Copy link
Member Author

Hi @charris, I won't be able to pursue this in the near future, feel free to close/reassign.

@mattip mattip closed this Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants