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

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

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 #19718

wants to merge 1 commit into from

Conversation

shubham11941140
Copy link
Contributor
@shubham11941140 shubham11941140 commented Aug 20, 2021 •
edited
Loading

The tobytes method of ndarray accepts 'K' for the order keyword argument, but appears not to keep the order as expected.

Reproducing Code Example:

>>> a = np.array([[1, 2], [3, 4]], dtype=np.uint8, order='F')
>>> a.ravel('K')   # Keep array order
array([1, 3, 2, 4])
>>> a.tobytes('K')  # Expect consistency with ravel
b'\x01\x02\x03\x04'

The above stated test case

This edit in the "convert.c" file actually solves the tobyte conversion issue. It parses the array and converts each value to the necessary byte value as expected.
Reviving #16328
"Fixes #16569"

@shubham11941140 shubham11941140 deleted the patch1 branch August 20, 2021 07:12
@shubham11941140 shubham11941140 restored the patch1 branch August 20, 2021 07:15
@shubham11941140 shubham11941140 changed the title BUG: lib: Fix handling of usecols=[] in loadtxt BUG: Fix order='K' behavior for tobytes Aug 20, 2021
@mattip
Copy link
Member
mattip commented Aug 20, 2021

At the very least this PR should have some text description

  • what problem does it solve
  • what issue is it related to

There should also be tests that fail before the PR and pass after

@shubham11941140
Copy link
Contributor Author

Sure, I will write a description.

@shubham11941140
Copy link
Contributor Author

There are no conflicts and all checks have passed, please merge it with the main, so I can close the Pull Request.

@mattip
Copy link
Member
mattip commented Aug 20, 2021

There are no conflicts and all checks have passed, please merge it with the main, so I can close the Pull Request.

That is not the way this works. Your PR will be reviewed. If there are no corrections or modifications needed (which is rare), and if it does not require further consideration, it may be merged. If it is not commented on for week, then you can ping with a reminder, but certainly not after 2 hours.

@mattip
Copy link
Member
mattip commented Aug 20, 2021

If the PR is meant to solve an issue, please use the magic incantation "Fixes #16569" so github will connect the two and close the issue if the PR is merged. Note how after I made this comment the "Linked issues" section on the right hand side now relates to that issue.

@shubham11941140
Copy link
Contributor Author

I think this should do, it is ready to be merged.

@mattip
Copy link
Member
mattip commented Aug 20, 2021

It is not ready to be reviewed. You need to add a test. I would suggest numpy/core/tests/test_multiarray.py, where tobytes is tested.

@shubham11941140
Copy link
Contributor Author
shubham11941140 commented Aug 20, 2021 •
edited
Loading

I added a test in numpy/core/tests/test_multiarray.py, but build error is coming. It is working on my local machine perfectly. I think we can go for the merge.

@shubham11941140
Copy link
Contributor Author

Please merge it with main.

@mattip
Copy link
Member
mattip commented Aug 20, 2021

Rather than requesting a merge, please request a review. It may be only a small semantic difference, but it shows you understand that reviewing code can be as hard as fixing code, and that it may take a few days or weeks for review to happen. Changes like these often have unwanted side effects that must be carefully considered before approval.

@mattip
Copy link
Member
mattip commented Aug 20, 2021

Where is the test?

@seberg
Copy link
Member
seberg commented Aug 20, 2021

This looks a lot like it is currently (mostly?) equivalent to gh-16328. Now that PR was possibly almost practically ready...

I had some pending comments there: 1. f (needs_api && PyErr_Occurred()) { is unnecessary, 2. to expand the tests with 3-D examples like x3d.tobytes("K") and x3d.transpose(1, 2, 0).tobytes("K"). But have to check whether there was another reason for the stalling, maybe I just thought fast-paths might be good.

@mattip
Copy link
Member
mattip commented Aug 21, 2021

Ahh, so I think we should close this and continue the work in #16328.

@mattip mattip added 50 - Duplicate 57 - Close? Issues which may be closable unless discussion continued labels Aug 21, 2021
@mattip mattip mentioned this pull request Aug 22, 2021
Closed
@shubham11941140
Copy link
Contributor Author

The code part that implements the tobytes is almost the same. This passes all the checks.
The previous PR solves the issue but the test fails.
The build is complete on my system and it works.

@shubham11941140
Copy link
Contributor Author

I committed it, so the branch has no conflicts with the base branch and it is ready to merge.

@eric-wieser
Copy link
Member
eric-wieser commented Aug 22, 2021 •
edited
Loading

This patch (77f8226) seems to match gh-16328 exactly. @shubham11941140, it is fine to copy code from stale PRs, but only if you make it clear what the original PR was and who the original author was (preferably with a Co-authored-by line or even by changing the author in the git commit).

In the likely event that your contribution is to fulfill part of a degree requirement, you are at risk of breaching plagiarism rules, which in many institutions including yours can be a serious offence that if repeated can lead to disqualification from your course:

At IIT Bhilai, plagiarism is strictly prohibited. A case of plagiarism will be dealt by DUGC/DPGC
and could be referred to the institute wide disciplinary action committee; the committee,
depending upon the severity of the case may give FS/F in the course, suspend the student for
a certain period or may expel the student from the institute. A faculty has the right to check
the students’ submission at any time and take necessary action. It is the responsibility of
students to ensure originality of their work, be aware of this policy and abide by it.

@shubham11941140
Copy link
Contributor Author

I can do that, but I am unaware of the process. Could you please do that?

@shubham11941140
Copy link
Contributor Author

Ok can you open it again, I will commit again with a different code. Please could you delete the plag comments.

@shubham11941140
Copy link
Contributor Author

Please reopen the PR as I cant open it

@shubham11941140
Copy link
Contributor Author

Please reopen it.

@shubham11941140
Copy link
Contributor Author

@eric-wieser please reopen the PR.

@shubham11941140
Copy link
Contributor Author

I have mentioned revival of PR.

@shubham11941140
Copy link
Contributor Author

Please remove the comments that involve the copy keyword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
00 - Bug 50 - Duplicate 57 - Close? Issues which may be closable unless discussion continued
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: order='K' behavior for tobytes
4 participants
@shubham11941140 @mattip @seberg @eric-wieser
0