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

Closed
anirudh2290 wants to merge 1 commit intonumpy:mainfrom
anirudh2290:fix_to_bytes_order_k
Closed

BUG: Fix order='K' behavior for tobytes#16328
anirudh2290 wants to merge 1 commit intonumpy:mainfrom
anirudh2290:fix_to_bytes_order_k

Conversation

@anirudh2290
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author
@anirudh2290 anirudh2290 May 21, 2020

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
Copy link
Copy Markdown
Member Author
@anirudh2290 anirudh2290 May 21, 2020

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
Copy Markdown
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 yo 8000 u would like to get to know that iterator API maybe not a bad exercise.

@seberg
Copy link
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Member Author
@anirudh2290 anirudh2290 Jun 11, 2020

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
Copy Markdown
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

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
Copy Markdown
Contributor
@shubham11941140 shubham11941140 Aug 22, 2021

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
Copy Markdown
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
Copy Markdown
Member
charris commented Jun 9, 2022

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

@anirudh2290
Copy link
Copy Markdown
Member Author
anirudh2290 commented Jun 10, 2022

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

+1 more reviewer

@shubham11941140 shubham11941140 shubham11941140 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

5422 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