Skip to content

BUG: Support sparse arrays in scipy.sparse.csgraph.laplacian#19156

Merged
perimosocordiae merged 2 commits intoscipy:mainfrom
bharatr21:sparse-laplace
Sep 6, 2023
Merged

BUG: Support sparse arrays in scipy.sparse.csgraph.laplacian#19156
perimosocordiae merged 2 commits intoscipy:mainfrom
bharatr21:sparse-laplace

Conversation

@bharatr21
Copy link
Copy Markdown
Contributor

@bharatr21 bharatr21 commented Aug 29, 2023

Reference issue

Fixes #19149

What does this implement/fix?

Resolves AttributeError raised on sparse arrays in scipy.sparse.csgraph.laplacian (See #19149)

Additional information

This resolves/unblocks test coverage in scikit-learn/scikit-learn#27161
Tagging @perimosocordiae for review, as he reviewed the original issue

Copy link
Copy Markdown
Contributor

@dschult dschult left a comment

Choose a reason for hiding this comment

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

For what it is worth, this looks good to me. :)

@perimosocordiae
Copy link
Copy Markdown
Member

Thanks for the PR! While we're here, could you add a unit test that exercises the bug?

@j-bowhay j-bowhay added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse.csgraph labels Aug 29, 2023
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Aug 29, 2023
@bharatr21
Copy link
Copy Markdown
Contributor Author

bharatr21 commented Sep 1, 2023

Thanks for the PR! While we're here, could you add a unit test that exercises the bug?

I need help with adding a test. I know where to add it - scipy/sparse/csgraph/tests/test_graph_laplacian.py
but I don't know whether to extend the existing tests or add a completely new one seeing this params/pattern repeated in many tests

@pytest.mark.parametrize("arr_type", [np.array,
                                      sparse.csr_matrix,
                                      sparse.coo_matrix])

cc @perimosocordiae

@dschult
Copy link
Copy Markdown
Contributor

dschult commented Sep 1, 2023

The arr_type parametrizing is the part of the test that should be extended. The existing _check... functions should not be affected by sparray input. And additional tests shouldn't be needed. So I think you should just add to the arr_type list with 2 more types: csr_array and coo_array. I think that shows up on lines 171, 247, 304. That should show that it breaks on the current system and is fixed by this PR.

@bharatr21 bharatr21 requested a review from dschult September 4, 2023 15:21
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.

Changes look good, and thanks for adding the extra test cases. LGTM

Copy link
Copy Markdown
Contributor

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Looks good to me too.
Thanks!

@bharatr21
Copy link
Copy Markdown
Contributor Author

What additional steps should be taken to get it merged or backport it to another version?

@perimosocordiae perimosocordiae merged commit 838a4e0 into scipy:main Sep 6, 2023
@perimosocordiae
Copy link
Copy Markdown
Member

Merged. Thanks!

@j-bowhay j-bowhay added this to the 1.12.0 milestone Sep 6, 2023
@tylerjereddy tylerjereddy modified the milestones: 1.12.0, 1.11.3 Sep 21, 2023
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Sep 21, 2023
…9156)

* BUG: Support sparse arrays in scipy.sparse.csgraph.laplacian

* TST: Added unit test for laplacian
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Sep 27, 2023
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.csgraph

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: scipy.sparse.csgraph.laplacian raises AttributeError on sparse arrays

6 participants