MAINT: fix type-narrowing addition in sparse.construct.bmat #14626
Conversation
|
I can confirm this would not raise anymore. Thanks. I will let @perimosocordiae have a look as I am not familiar with these. |
tupui
added
defect
scipy/sparse/construct.py
Outdated
| row[idx] = B.row + row_offsets[i] | ||
| col[idx] = B.col + col_offsets[j] | ||
| row[idx] = np.dtype(idx_dtype).type(B.row) + row_offsets[i] | ||
| col[idx] = np.dtype(idx_dtype).type(B.col) + col_offsets[j] |
There was a problem hiding this comment.
This casting is a bit strange looking to my eye. How about this:
np.add(B.row, row_offsets[i], out=row[idx], dtype=idx_dtype)
np.add(B.col, col_offsets[i], out=col[idx], dtype=idx_dtype)
This is a bit more explicit about what's happening regarding the dtypes, and may be slightly more efficient as well.
There was a problem hiding this comment.
Agreed that the original change here looks a little odd. @kevinrichardgreen could you make this change and resolve the merge conflict? (if you don't know how to do that, please let me know and I'll help)
There was a problem hiding this comment.
Changed. Though the merge really does seem to be a mess right now. I've established that this just need to be moved into _construct.py. I can force push my local branch (that is rebased off of scipy/master) that has the desired version to kevinrichardgreen:fix-bmat so that it can be accepted from this PR. (Sidenote: do forced pushes mess with PRs in any substantial way?)
There was a problem hiding this comment.
Thanks @kevinrichardgreen!
Sidenote: do forced pushes mess with PRs in any substantial way?
No, that is perfectly fine. A PR always just reflects whatever the state is of the remote branch that the PR was originally made from. You can always rewrite history however you like.
|
I agree it's not going to be easy to test this, so for now I think it's fine to omit a reproducing unit test. |
kevinrichardgreen
force-pushed
the
fix-bmat
branch
from
5effd06
to
37aceb6
Compare
rgommers
changed the title
There was a problem hiding this comment.
This LGTM, and follows the recommendation of @perimosocordiae. So in it goes. Thanks both!
Brought up in issue #14623
Note I have not added a unit test for this as the to catch the overflow in the situation described in the issue requires >20GB of RAM. If there is some suggestion as to how such a large test could be incorporated (basically directly from the mwe in the issue), please advise.