GH-50906: [Python] Reshape 1D tensors to 2D in SparseCSR/CSC matrix conversion - #50907
GH-50906: [Python] Reshape 1D tensors to 2D in SparseCSR/CSC matrix conversion#50907pratyushadk wants to merge 1 commit into
Conversation
|
|
fd4c81d to
0e17ebe
Compare
|
@pratyushadk I've renamed the issue and the PR to reflect that this should be implemented in Python. Let's move the discussion here. |
0e17ebe to
cccd951
Compare
|
@rok Thanks for the direction. I'm new to open source and your feedback really helped me understand where the fix actually belongs. Updated the PR with the Python reshape approach. Looking forward to contributing more in the future! |
cccd951 to
6c7f030
Compare
ab6a7e0 to
3e02492
Compare
rok
left a comment
There was a problem hiding this comment.
@pitrou we've gone through several iterations here. Please review and feel free to merge.
@pratyushadk I understand you're trying to contribute and I appreciate it. But in the future please open issues and PRs with clear motivations as you're bound to waste time otherwise.
| array = np.array([1, 0, 2, 0, 0, 3, 0, 4], dtype=np.int64) | ||
| tensor = pa.Tensor.from_numpy(array) | ||
|
|
||
| scipy_array = sc_array_class(array.reshape(1, -1)) |
There was a problem hiding this comment.
Can we change this?
| scipy_array = sc_array_class(array.reshape(1, -1)) | |
| scipy_array = sc_array_class(array) |
There was a problem hiding this comment.
Thank you for the review and guidance, @rok. I'll make sure to open issues and PRs with clearer motivations in future. Regarding the suggested change, csc_array(array) raises ValueError: CSC arrays don't support 1D input. Use 2D, so the reshape(1, -1) is needed to construct both csr_array and csc_array with matching (1, n) shape.
There was a problem hiding this comment.
I see. This is annoying - so:
>>> scipy.sparse.csc_array([1, 0, 2, 0, 0, 3]).ndim
ValueError: CSC arrays don't support 1D input. Use 2D
>>> scipy.sparse.csc_matrix([1, 0, 2, 0, 0, 3]).ndim
2csc_matrix accepts 1D input while csc_array array does not.
Let's then simply test that 1D csc_matrix/csr_matrixroundtrips PyArrow tensor and that PyArrow tensor roundtrips to csc_matrix/csr_matrix. Forbid reshape path in case we have another type of object.
There was a problem hiding this comment.
Sure. I've switched the test to verify both roundtrips with csr_matrix and csc_matrix:
1D scipy matrix -> PyArrow -> compare dense1D PyArrow tensor -> sparse -> to_scipy -> compare to scipy matrix
3e02492 to
ebd7585
Compare
| if obj.ndim == 1: | ||
| obj = Tensor.from_numpy(obj.to_numpy().reshape(1, obj.shape[0])) |
There was a problem hiding this comment.
I want to move these from from_tensor to from_scipy and only do this if we're coming from scipy.{csc,csr}_matrix and reject otherwise. scipy is moving away from allowing 1D input here for a reason.
There was a problem hiding this comment.
That makes sense, reverted the from_tensor changes. I've added validation in from_scipy to reject non-2D input (previously from_scipy(csr_array(1d_array)) would silently create an invalid 1D SparseCSRMatrix). The test now verifies the from_scipy roundtrip with csr_matrix/csc_matrix, which normalize 1D to (1, n).
ebd7585 to
4fbb5a2
Compare
Rationale for this change
SparseCSRMatrixandSparseCSCMatrixrepresent 2-dimensional sparse matrices. In the current codebase, passing a 1D SciPy sparse array (such asscipy.sparse.csr_array([1, 0, 2])withndim == 1) toSparseCSRMatrix.from_scipyorSparseCSCMatrix.from_scipysilently creates a 1DSparseCSRMatrixwith shape(n,)without dimension validation.This PR adds explicit 2D dimension checks to
from_scipyinSparseCSRMatrixandSparseCSCMatrixto raise aValueErrorfor non-2D sparse objects, while ensuring legacycsr_matrix/csc_matrixobjects (which normalize 1D input to(1, n)) continue to work and are tested.What changes are included in this PR?
python/pyarrow/tensor.pxi: Addedif obj.ndim != 2:validation inSparseCSRMatrix.from_scipyandSparseCSCMatrix.from_scipyto raiseValueError("Expected 2-dimensional sparse input for ...").python/pyarrow/tests/test_sparse_tensor.py: Added a parametric unit testtest_sparse_csx_matrix_from_1dverifying that 1D vector input passed viacsr_matrix/csc_matrixproduces the expected(1, n)2D sparse matrix viafrom_scipy.Are these changes tested?
Yes, tested by the new parametric unit test
test_sparse_csx_matrix_from_1dinpython/pyarrow/tests/test_sparse_tensor.py.Are there any user-facing changes?
Yes.
SparseCSRMatrix.from_scipyandSparseCSCMatrix.from_scipynow explicitly raiseValueErrorif passed a 1D sparse array instead of silently creating an invalid 1D sparse matrix.