Skip to content

BUG: sparse: Clean up 1D input handling to sparse array/matrix and add tests#20444

Merged
perimosocordiae merged 4 commits intoscipy:mainfrom
dschult:fix_1d_to_2d_init
May 9, 2024
Merged

BUG: sparse: Clean up 1D input handling to sparse array/matrix and add tests#20444
perimosocordiae merged 4 commits intoscipy:mainfrom
dschult:fix_1d_to_2d_init

Conversation

@dschult
Copy link
Copy Markdown
Contributor

@dschult dschult commented Apr 11, 2024

Fixes #20415
1D input to sparse matrix constructor creates a 2D matrix with one row. This could be thought of as a bug (1D input should not create 2D output) or a feature (1D input saves an extra square bracket pair of typing). But it definitely needs to be changed when moving to a sparse array api that supports 1D sparse arrays.

In addition, the 1D input case is not handled gracefully in 1.13 for csr_array because the CSR 1D support PR did not get in in time.

The 1D input case to 2D output does have a test for sparse matrices, but not for arrays. So new tests are needed as well. The module test_base.py even uses 1D input in 2 places -- this PR changes those to 2D input. The test for the 1D input -> 2D output behavior is left as is. The test for arrays depends on format and in this PR is added to test_common1d.py updated to check arrays as well as matrices that the FutureWarning is in place.

This PR also adds tests to ensure behavior is determined for 1D input for all formats. It improves the ValueError message for the csr_array case to adjust the reported confusing ValueError message (which will become moot when the csr PR is merged). It adds FutureWarnings for sparse matrix when 1D constructor input occurs in all formats. It adds ValueErrors with better error messages for sparse formats that are not planned to support 1D arrays.

The sparse working group should discuss this on Monday to ensure no other similar cases have been missed.

Precommit linting required some changes which are not directly related to the other content in this PR.

@github-actions github-actions bot added scipy.sparse defect A clear bug or issue that prevents SciPy from being installed or used as expected labels Apr 11, 2024
@dschult dschult changed the title BUG: sparse: Clean up 1D constructor input to sparse array/matrix and add tests BUG: sparse: Clean up 1D input handling to sparse array/matrix and add tests Apr 11, 2024
@perimosocordiae
Copy link
Copy Markdown
Member

I think the big question to resolve here is: should spmatrix types continue to support construction from 1d dense inputs?

@dschult
Copy link
Copy Markdown
Contributor Author

dschult commented Apr 30, 2024

In the April 15 sparse arrays meeting it was pointed out that np.matrix also constructs 2D matrix from 1D input. So we should leave spmatrix behavior as it is. I've updated this PR to only change behavior for sparray. It raises a ValueError when format does not support 1D otherwise it creates a 1D sparse array.

For test_base.py it changes a few tests which use 1D input to use 2D input and leaves the explicit test of 1D input as is for spmatrix. The 1D input for sparray is tested in test_common1d.py

@perimosocordiae can you take another look at this one?

@dschult dschult force-pushed the fix_1d_to_2d_init branch from 968c6a3 to d590d02 Compare May 8, 2024 03:43
@dschult dschult force-pushed the fix_1d_to_2d_init branch from d590d02 to 2390855 Compare May 8, 2024 03:52
Copy link
Copy Markdown
Member

@perimosocordiae perimosocordiae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors will be much nicer for users than what we currently raise. Thanks!

@perimosocordiae perimosocordiae merged commit de0e3e3 into scipy:main May 9, 2024
@dschult dschult deleted the fix_1d_to_2d_init branch May 9, 2024 13:56
@lucascolley lucascolley added this to the 1.14.0 milestone May 9, 2024
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 9, 2024
@tylerjereddy
Copy link
Copy Markdown
Contributor

This fixes a complaint about 1.13.0, so I've tentatively added a backport label. Whether it actually gets backported depends on the situation with merge conflicts, since sparse has been fairly active.

@tylerjereddy tylerjereddy modified the milestones: 1.14.0, 1.13.1 May 15, 2024
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request May 15, 2024
…d tests (scipy#20444)

* test 1d input to sparse. Add FutureWarnings and ValueErrors

* remove matrix changes. Let them create 2D

* correct imports

* rebase on main and update to support 1D CSR input
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: csr_array can no longer be initialized with 1D array

4 participants