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

diag = >diag.copy() when the matrix will be manipulated. #350

Merged
merged 1 commit into from Aug 31, 2015
Merged

diag = >diag.copy() when the matrix will be manipulated. #350

merged 1 commit into from Aug 31, 2015

Conversation

bcipolli
Copy link
Contributor

This is a general fix for #325 . In the newest version of numpy, when diag returns a vector of numbers along the diagonal, that vector is read-only. In places where we wish to manipulate the elements of that vector, we need to call copy() first.

I manually went through to check each call to diag, and to see where we manipulated the elements or returned the vector value.

Questions:

@matthew-brett
Copy link
Member

The travis tests use latest numpy, as well as minimal numpy - is that enough?

If you feel like adding a plotting regression test, that would be very helpful.

@bcipolli
Copy link
Contributor Author

OK, then we should beef up the test coverage before merging this PR--there should be multiple issues, not just the plotting one.

I'll take a look at that now.

@bthirion
Copy link
Contributor

This PR looks good.

@matthew-brett
Copy link
Member

Ben - where are we with this one?

@bcipolli
Copy link
Contributor Author

Oops, I got sidetracked. I determined two of the changes were unnecessary, and that two of the changes are untested. I simply need to write tests for them.

I will do that now.

@bcipolli
Copy link
Contributor Author

@matthew-brett most of the changes here were unnecessary. #325 had been fixed elsewhere, most of the changes I made here turned out to be irrelevant.

What's left is one final change, and it's basically untestable--requires Mayavi to be installed. So.. I think just take it as-is or dump it.

@matthew-brett
Copy link
Member

Seems to have another PR In list of commits? Bad rebase maybe?

@bcipolli
Copy link
Contributor Author

Re- rebased. Travis build is looking fine.

@matthew-brett
Copy link
Member

Nice, thanks.

matthew-brett added a commit that referenced this pull request Aug 31, 2015
MRG: diag = >diag.copy() when the matrix will be manipulated

diag has become a view in latest numpy; do copy where values need to be have write access.
@matthew-brett matthew-brett merged commit c066f0f into nipy:master Aug 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants