BUG: sparse: fix bugs in dia_matrix.setdiag #3272

Merged
merged 1 commit into from Feb 25, 2014

Conversation

Projects
None yet
3 participants
@pv
Owner

pv commented Feb 2, 2014

Some bugs in dia_matrix.setdiag slipped through. This PR fixes:

  • hide details of the internal dia_matrix data representation in
    setdiag(), so that setdiag() behavior is same for all spmatrix types
  • fix bugs in resizing self.data, which doesn't always have
    self.shape[1]==N
  • add scalar broadcasting behavior to setdiag() for all matrix types
  • add comprehensive tests

(Milestone added --- we don't want to have setdiag with known bugs there...)

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 2, 2014

Coverage Status

Coverage remained the same when pulling 018181d on pv:fix-setdiag into 28f9d10 on scipy:master.

Coverage Status

Coverage remained the same when pulling 018181d on pv:fix-setdiag into 28f9d10 on scipy:master.

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Feb 14, 2014

Owner

Will merge soon-ish if no comments, so it makes 0.14.0

Owner

pv commented Feb 14, 2014

Will merge soon-ish if no comments, so it makes 0.14.0

BUG: sparse: fix bugs in dia_matrix.setdiag
- hide details of the internal dia_matrix data representation in
  setdiag(), so that behavior matches the other spmatrix types

- fix bugs in resizing self.data, which doesn't always have
  self.shape[1]==N

- add scalar broadcasting behavior to setdiag() for all matrix types
@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 19, 2014

Coverage Status

Coverage remained the same when pulling 4cd67a6 on pv:fix-setdiag into 96c85a5 on scipy:master.

Coverage Status

Coverage remained the same when pulling 4cd67a6 on pv:fix-setdiag into 96c85a5 on scipy:master.

@pv pv added the PR label Feb 19, 2014

@rgommers

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 25, 2014

Owner

This increases the number of errors on 32-bit:

======================================================================
ERROR: test_base.TestLIL.test_setdiag
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rgommers/.local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/rgommers/Code/scipy/scipy/sparse/tests/test_base.py", line 646, in test_setdiag
    check_setdiag(a, b, k)
  File "/home/rgommers/Code/scipy/scipy/sparse/tests/test_base.py", line 624, in check_setdiag
    b.setdiag(v, k)
  File "/home/rgommers/Code/scipy/scipy/sparse/base.py", line 770, in setdiag
    self[i, i + k] = values
  File "/home/rgommers/Code/scipy/scipy/sparse/lil.py", line 286, in __setitem__
    i, j, x)
  File "_csparsetools.pyx", line 112, in scipy.sparse._csparsetools.__pyx_fused_cpdef (../scipy/sparse/_csparsetools.c:3578)
TypeError: No matching signature found

Probably they'll be fixed when fixed the existing errors, but let's hold off on this one just to be sure.

Owner

rgommers commented Feb 25, 2014

This increases the number of errors on 32-bit:

======================================================================
ERROR: test_base.TestLIL.test_setdiag
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rgommers/.local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/rgommers/Code/scipy/scipy/sparse/tests/test_base.py", line 646, in test_setdiag
    check_setdiag(a, b, k)
  File "/home/rgommers/Code/scipy/scipy/sparse/tests/test_base.py", line 624, in check_setdiag
    b.setdiag(v, k)
  File "/home/rgommers/Code/scipy/scipy/sparse/base.py", line 770, in setdiag
    self[i, i + k] = values
  File "/home/rgommers/Code/scipy/scipy/sparse/lil.py", line 286, in __setitem__
    i, j, x)
  File "_csparsetools.pyx", line 112, in scipy.sparse._csparsetools.__pyx_fused_cpdef (../scipy/sparse/_csparsetools.c:3578)
TypeError: No matching signature found

Probably they'll be fixed when fixed the existing errors, but let's hold off on this one just to be sure.

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Feb 25, 2014

Owner

The alternatives are to merge this before 0.14.0, or to revert eef58eb

Owner

pv commented Feb 25, 2014

The alternatives are to merge this before 0.14.0, or to revert eef58eb

@rgommers

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 25, 2014

Owner

Yes, I left the 0.14.0 milestone on this PR on purpose. We should fix those 32-bit issues anyway and then merge this.

Owner

rgommers commented Feb 25, 2014

Yes, I left the 0.14.0 milestone on this PR on purpose. We should fix those 32-bit issues anyway and then merge this.

@rgommers

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 25, 2014

Owner

Since this in itself is OK, let's merge it.

Owner

rgommers commented Feb 25, 2014

Since this in itself is OK, let's merge it.

rgommers added a commit that referenced this pull request Feb 25, 2014

Merge pull request #3272 from pv/fix-setdiag
BUG: sparse: fix bugs in dia_matrix.setdiag

@rgommers rgommers merged commit c43260b into scipy:master Feb 25, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment