8000 Make a.diagonal() writeable. · Issue #5407 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Make a.diagonal() writeable. #5407

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
Open
charris opened this issue Jan 1, 2015 · 16 comments
Open

Make a.diagonal() writeable. #5407

charris opened this issue Jan 1, 2015 · 16 comments
Labels
17 - Task 54 - Needs decision

Comments

@charris
Copy link
Member
charris commented Jan 1, 2015

The ndarray.diagonal method currently returns a view, but it is not writeable. It is documented to return a writeable view in 1.10.
@charris charris added this to the 1.10 blockers milestone Jan 1, 2015
@maniteja123
Copy link
Contributor

Can I attempt to complete this task ? I am not completely accustomed to the core of numpy but it would be nice if I can try to go through the code for understanding a bit of core python too, since I think this is the line that needs to be removed for the view to be read/write ( as commented in code)
https://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/item_selection.c#L2193

I had found this by searching for PyArray_Diagonal using grep. So, it would be helpful if someone would guide how to find the corresponding core function , say ravel, diagonal or clip.

@charris
Copy link
Member Author
charris commented Jan 1, 2015

Sure, have at it. Looks like you found the right spot. There should 8000 be a test somewhere that also needs to be updated. If not, you can add one :)

@maniteja123
Copy link
Contributor

@charris Thanks :) I will also check if there is corresponding test regarding the diagonal command.

@maniteja123
Copy link
Contributor

I think this is the test to check the writability of the view returned by the diagonal. https://github.com/numpy/numpy/blob/master/numpy/core/tests/test_multiarray.py#L1738

The change needs to be in the assert statement of the a.flags.writeable flag, which needs to be inverted, but the change for the a.flags.owndata need not be changed, I suppose. Do correct me if I am wrong.

@charris
Copy link
Member Author
charris commented Jan 1, 2015

Yep, that is the place and the changes you suggest look right. Don't forget to rename the test function.

@maniteja123
Copy link
Contributor

Yeah, sure. When we change the name of the test function, do we need to update anywhere ?

@maniteja123 maniteja123 mentioned this issue Jan 1, 2015
Closed
@charris
Copy link
Member Author
charris commented Jan 1, 2015

Should not matter, nose picks up the test functions by their prefix test.

@maniteja123
Copy link
Contributor

Oh thanks , just wanted to confirm. I referenced it in the PR #5409 .

@maniteja123
Copy link
Contributor

@charris, I wanted to try out the new changes proposed, it looks like the PyArray_Diagonal needs to be reverted to the scenario before 1.9.0, where a copy was created and returned.here. Though there was a FutureWarning raised then, I think it should be a different scenario now.
Also, it would be great if you could suggest whether implementing this whole task is feasible for a newcomer like me. If yes, I would want to give it a try to implement the new feature.

@njsmith
Copy link
Member
njsmith commented Jan 16, 2015

@maniteja123: Honestly, I'd stay clear of this one as a newcomer -- it's ~5% programming and 95% community debate wrangling.

@maniteja123
Copy link
Contributor

@njsmith I do understand the implications of this feature. I just wanted to clarify. Thanks :)

@charris charris modified the milestones: 1.11 blockers, 1.10 blockers Jun 21, 2015
@anntzer
Copy link
Contributor
anntzer commented Sep 28, 2015

Gently bumping the issue. I assume the issue has been indeed moved to 1.11 but the docstring needs some updating in that case. (I would also reorder the sections of the docstring so that "current behavior" rather than "past behavior" goes to the top.)

@njsmith njsmith modified the milestones: 1.12.0 release, 1.11.0 blockers Jan 15, 2016
@njsmith
Copy link
Member
njsmith commented Jan 15, 2016

Moving to 1.12, since we haven't talked about this and 1.11 is imminent. Docstrings have been fixed in master to state that this is a planned-at-some-indefinite-future-date rather than giving a specific version.

@jakirkham
Copy link
Contributor

Seems like it would be a nice feature some day. Especially since it was already documented as a feature. What's the "community debate wrangling" here? Should I even ask? ;)

@charris
Copy link
Member Author
charris commented Jan 17, 2016

IIRC, there is a failing test. The transition was from returning a copy of the diagonal to returning a view, that's why the initial step was to make the view ro so folks would not accidentally change the original array.

@rgommers
Copy link
Member

What's the "community debate wrangling" here? Should I even ask? ;)

It's good to ask. There was a long debate on how bad the backwards compatibility break was: http://article.gmane.org/gmane.comp.python.numeric.general/59620. In hindsight we probably never should have made any changes. It's still a bit up in the air what we'll do now and when I think.

@ahaldane ahaldane mentioned this issue Sep 20, 2016
Merged
@rgommers rgommers removed this from the 1.12.0 release milestone Feb 15, 2017
@rgommers rgommers removed this from the 1.12.0 release milestone Feb 15, 2017
@ahaldane ahaldane mentioned this issue Mar 24, 2019
Merged
@rgommers rgommers mentioned this issue May 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
17 - Task 54 - Needs decision
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@charris @rgommers @njsmith @anntzer @jakirkham @maniteja123
0