8000 BUG: Fix order='K' behavior for tobytes by anirudh2290 · Pull Request #16328 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

anirudh2290
Copy link
Member
@anirudh2290 anirudh2290 commented May 21, 2020 •
edited by rossbar
Loading

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

@anirudh2290 anirudh2290 force-pushed the fix_to_bytes_order_k branch from 2756323 to 9fbc4ba Compare May 21, 2020 17:56
anirudh2290
anirudh2290 commented May 21, 2020
View reviewed changes
numpy/core/src/multiarray/convert.c Outdated
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 ?

numpy/core/src/multiarray/convert.c Outdated
|| (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.

anirudh2290
anirudh2290 commented May 21, 2020
View reviewed changes
This was referenced Jun 10, 2020
Merged
Open
@anirudh2290 anirudh2290 force-pushed the fix_to_bytes_order_k branch 2 times, most recently from 717c597 to f2b6d3f Compare June 11, 2020 19:43
anirudh2290
anirudh2290 commented Jun 11, 2020
View reviewed changes
numpy/core/src/multiarray/convert.c
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.

@anirudh2290 anirudh2290 force-pushed the fix_to_bytes_order_k branch from f2b6d3f to 6cdfcc9 Compare June 11, 2020 20:42
Base automatically changed from master to main March 4, 2021 02:04
@seberg seberg mentioned this pull request Aug 20, 2021
Closed
shubham11941140
shubham11941140 reviewed Aug 22, 2021
View reviewed changes
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.

numpy/core/tests/test_multiarray.py
@@ -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
Reviewers

@seberg seberg seberg left review comments

@shubham11941140 shubham11941140 shubham11941140 left review comments

Assignees
No one assigned
Labels
00 - Bug component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anirudh2290 @seberg @charris @shubham11941140 @mattip
0