FIX Enables label_ranking_average_precision_score to support sparse y_true #23442

ShehanAT · 2022-05-22T16:33:38Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Since the label_ranking_average_precision_score metric now uses the check_array function(Link to relevant commit) in favour of the previously used check_arrays function(Link to relevant commit), passing a None value for the sparse_format parameter now raises a TypeError: A sparse matrix was passed, but dense data is required. Use X.toarray() to convert to a dense numpy array error.

This PR changes the value of the sparse_format parameter so that whenever it is passed a None value, it is automatically assigned a value of ["csr", "csc"]. Doing so prevents the aforementioned error from being raised.

I have added a corresponding test for this fix in the following file: sklearn/metrics/tests/test_ranking.py, # def test_label_ranking_avg_precision_score_should_allow_csr_matrix_for_y_true_input()

I have ran the pytest sklearn/metrics/tests/test_ranking.py command for which all the tests, including the newly added one, have passed.

thomasjpfan

Thank you for the PR!

thomasjpfan · 2022-05-22T17:40:09Z

sklearn/utils/validation.py

+        if accept_sparse is False:
+            accept_sparse = ["csr", "csc"]


We can not do this because it changes the behavior of accept_sparse in check_array and is backward incompatible, which is causing many CI failures.

To fix this issue, we need to adjust label_ranking_average_precision_score itself to accept y_true as sparse, by setting accept_sparse="csr" here:

scikit-learn/sklearn/metrics/_ranking.py

Line 1073 in 735d3c6

y_true = check_array(y_true, ensure_2d=False)

From looking at the code, it maybe a little difficult to do since type_of_target does not really work with sparse targets. The quick fix is to adjust label_ranking_average_precision_score as follows:

y_true = check_array(y_true, ensure_2d=False, accept_sparse="csr") ... if not issparse(y_true): # Handle badly formatted array and the degenerate case with one label y_type = type_of_target(y_true, input_name="y_true") if y_type != "multilabel-indicator" and not ( y_type == "binary" and y_true.ndim == 2 ): raise ValueError("{0} format is not supported".format(y_type)) y_true = csr_matrix(y_true)

Thanks for the feedback. I have included this change by adding the if not issparse(y_true): line in a subsequent commit now. Let me know if I'm missing anything.

sklearn/metrics/tests/test_ranking.py

…g 'if not issparse(y_true)' line

…py ln ~815

sklearn/metrics/tests/test_ranking.py

thomasjpfan · 2022-05-23T16:32:23Z

sklearn/metrics/tests/test_ranking.py

+    y_true = csr_matrix(np.array([[1, 0, 0], [0, 0, 1]]))
+    y_score = np.array([[0.5, 0.9, 0.6], [0, 0, 1]])
+    result = label_ranking_average_precision_score(y_true, y_score)
+    assert result == pytest.approx(0.6666666666666666)


Nit: This should work and looks a little bit better:

Suggested change

assert result == pytest.approx(0.6666666666666666)

assert result == pytest.approx(2/3)

Edit: Changed 1/3 to 2/3

The 1/3 value should be 2/3 right?

Yea it's 2/3, I mistyped.

thomasjpfan · 2022-05-23T16:32:23Z

sklearn/utils/validation.py

@@ -811,6 +811,7 @@ def check_array(

    if sp.issparse(array):
        _ensure_no_complex_data(array)
+


This diff can be reverted.

…; reverted unwanted diffs

thomasjpfan

Merging with main should fix the ci failure.

Please add an entry to the change log at doc/whats_new/v1.2.rst with tag |Fix|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

thomasjpfan · 2022-05-24T16:55:52Z

sklearn/metrics/_ranking.py

@@ -37,6 +37,7 @@
 from ..preprocessing import label_binarize
 from ..utils._encode import _encode, _unique

+


This diff is not needed.

Ok, removed now

thomasjpfan · 2022-05-24T16:55:52Z

sklearn/metrics/_ranking.py

+        if y_type != "multilabel-indicator" and not (
+            y_type == "binary" and y_true.ndim == 2
+        ):
+            raise ValueError("{0} format is not supported".format(y_type))

    y_true = csr_matrix(y_true)


We only need to convert if y_true is dense:

Suggested change

y_true = csr_matrix(y_true)

y_true = csr_matrix(y_true)

check_array with accept_sparse="csr" will convert a sparse matrix to CSR if it needs to:

scikit-learn/sklearn/utils/validation.py

Lines 641 to 642 in 996c7e2

'csr', etc. If the input is sparse but not in the allowed format,

it will be converted to the first listed format. True allows the input

I see, fixed now

ShehanAT · 2022-05-24T19:03:50Z

Merging with main should fix the ci failure.

Please add an entry to the change log at doc/whats_new/v1.2.rst with tag |Fix|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

I have added an entry to doc/whats_new/v1.2.rst now. Let me know if I'm missing anything else.

thomasjpfan

Minor comment, otherwise LGTM.

I'm okay with this temporary solution, because type_of_target does not support sparse data yet.

doc/whats_new/v1.2.rst

sklearn/metrics/tests/test_ranking.py

Co-authored-by: Thomas J. Fan <[email protected]>

cmarmo · 2022-08-15T20:28:06Z

Hi @ShehanAT, you already have one approval, do you mind synchronizing with upstream and fixing conflicts?
This will rerender the documentation and probably fix the failing builds, making easier to have new reviewers. Thanks!

ShehanAT · 2022-08-16T13:01:14Z

Hi @ShehanAT, you already have one approval, do you mind synchronizing with upstream and fixing conflicts? This will rerender the documentation and probably fix the failing builds, making easier to have new reviewers. Thanks!

Sure, I have merged and fixed conflicts involving the doc/whats_new/v1.2.rst file now.

Is it okay that all the recent upstream main commits got lumped into this PR or do you want me to create a new PR with the same changes?

cmarmo · 2022-08-16T18:27:49Z

Thanks @ShehanAT. Upstream commits should not be in the history of your PR. A new PR is not needed.
Could you please synchronize your main then rebase your branch against main?

$ git rebase main

Note that some relics of conflicts (>>>> HEAD) are still present: they should be removed before pushing, otherwise the build will not complete.

Let me know if you need any help in the process.

ShehanAT · 2022-08-18T00:16:05Z

Thanks @ShehanAT. Upstream commits should not be in the history of your PR. A new PR is not needed. Could you please synchronize your main then rebase your branch against main?
$ git rebase main
Note that some relics of conflicts (>>>> HEAD) are still present: they should be removed before pushing, otherwise the build will not complete.

Let me know if you need any help in the process.

Hi @cmarmo. Thanks for the guidance but the rebasing with main is still being identified as new commits for some reason. What you do think is the most convenient way to resolve this issue?

cmarmo · 2022-08-18T04:04:16Z

Hi @ShehanAT the last commit before the synchronization is a29b92d.
I suggest you to reset the branch at this commit.

$ git reset --hard a29b92da1b1501b27ad147997b24dda28ea5b792

Then redo the synchronization

$ git fetch upstream
$ git merge upstream/main

You will have conflicts in the v1.2.rst file and only there (I have just done the workflow myself locally): once the conflicts fixed

$ git add doc/whats_new/v1.2.rst
$ git commit -m "Fix conflicts."

At the end of the process, you might want to check if everything went fine with

$ git diff upstream/main --compact-summary

which should output (at the time of me writing this)

 doc/whats_new/v1.2.rst                |  4 ++++
 sklearn/metrics/_ranking.py           | 18 ++++++++++--------
 sklearn/metrics/tests/test_ranking.py |  9 +++++++++
 3 files changed, 23 insertions(+), 8 deletions(-)

The three files you modified if I am not mistaken.
To push run:

$ git push --force <your_remote> <your_branch>

ShehanAT · 2022-08-18T21:11:12Z

Hi @cmarmo, thanks for the instructions.

My main branch is synchronized and my feature branch is rebased against the main branch now.

Let me know if any further changes are needed.

doc/whats_new/v1.2.rst

Co-authored-by: Chiara Marmo <[email protected]>

…trix-fix-ShehanAT

cmarmo · 2022-08-19T22:40:46Z

All green and up-to-date! Thanks @ShehanAT. Hoping that a second review will arrive soon... 🤞

sklearn/metrics/_ranking.py

Co-authored-by: Christian Lorentzen <[email protected]>

lorentzenchr

LGTM
I guess we can rely on type_of_target to be tested.

…_true (scikit-learn#23442) Allow y_true to be in CSR format.

ShehanAT added 2 commits May 22, 2022

reverted sklearn/metrics/_ranking.py to original content

d6eff79

Merge remote-tracking branch 'upstream/main' into main

576ab07

github-actions bot added module:metrics module:utils labels May 22, 2022

ShehanAT changed the title ~~Label ranking average precision score csr matrix fix shehan at~~ Label_ranking_average_precision_score Does Not Allow csr_matrix for y_true Input Fix, Regarding Issue #22575 May 22, 2022

ShehanAT changed the title ~~Label_ranking_average_precision_score Does Not Allow csr_matrix for y_true Input Fix, Regarding Issue #22575~~ [MRG] Label_ranking_average_precision_score Does Not Allow csr_matrix for y_true Input Fix, Regarding Issue #22575 May 22, 2022

thomasjpfan reviewed May 22, 2022

View changes

ShehanAT added 2 commits May 23, 2022

made changes to label_ranking_average_precision_score metric by addin…

fb5cb58

…g 'if not issparse(y_true)' line

removed redundant comment from scikit-learn/sklearn/utils/validation.…

aa67c76

…py ln ~815

thomasjpfan reviewed May 23, 2022

View changes

ShehanAT added 2 commits May 24, 2022

added docstring to test in sklearn/metrics/tests/test_ranking.py file…

6de41d4

…; reverted unwanted diffs

added docstring to test in sklearn/metrics/tests/test_ranking.py file…

dd977f2

…; reverted unwanted diffs

thomasjpfan reviewed May 24, 2022

View changes

added entry to doc/whats_new/v1.2.rst, removed redundant diffs

4313e68

editted entry to doc/whats_new/v1.2.rst

799aa1c

thomasjpfan changed the title ~~[MRG] Label_ranking_average_precision_score Does Not Allow csr_matrix for y_true Input Fix, Regarding Issue #22575~~ FIX Enables label_ranking_average_precision_score to support sparse y_true May 24, 2022

thomasjpfan approved these changes May 24, 2022

View changes

doc/whats_new/v1.2.rst Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_ranking.py Outdated Show resolved Hide resolved

ShehanAT and others added 2 commits May 24, 2022

Update sklearn/metrics/tests/test_ranking.py

9578f38

Co-authored-by: Thomas J. Fan <[email protected]>

Update doc/whats_new/v1.2.rst

a29b92d

Co-authored-by: Thomas J. Fan <[email protected]>

cmarmo added the Waiting for Reviewer label Aug 10, 2022

fixed conflicts in doc/whats_new/v1.2.rst

b383b9d

ShehanAT force-pushed the label-ranking-average-precision-score-csr-matrix-fix-ShehanAT branch from 499fd06 to b383b9d Compare Aug 18, 2022

cmarmo reviewed Aug 18, 2022

View changes

doc/whats_new/v1.2.rst Outdated Show resolved Hide resolved

ShehanAT and others added 2 commits Aug 19, 2022

Update doc/whats_new/v1.2.rst

2a45e24

Co-authored-by: Chiara Marmo <[email protected]>

Merge branch 'main' into label-ranking-average-precision-score-csr-ma…

4ff0c8e

…trix-fix-ShehanAT

lorentzenchr requested changes Aug 20, 2022

View changes

sklearn/metrics/_ranking.py Outdated Show resolved Hide resolved

Update sklearn/metrics/_ranking.py

2b56a6d

Co-authored-by: Christian Lorentzen <[email protected]>

ShehanAT requested a review from lorentzenchr Aug 21, 2022

cmarmo removed the Waiting for Reviewer label Aug 21, 2022

lorentzenchr approved these changes Sep 5, 2022

View changes

lorentzenchr merged commit 272c197 into scikit-learn:main Sep 5, 2022
32 checks passed

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 12, 2022

FIX Enables label_ranking_average_precision_score to support sparse y…

b149b0e

…_true (scikit-learn#23442) Allow y_true to be in CSR format.

Dec	JAN	Apr
	28
2022	2023	2025

FIX Enables label_ranking_average_precision_score to support sparse y_true #23442

FIX Enables label_ranking_average_precision_score to support sparse y_true #23442

ShehanAT commented May 22, 2022

thomasjpfan left a comment

thomasjpfan May 22, 2022

ShehanAT May 23, 2022 •

edited

thomasjpfan May 23, 2022 •

edited

ShehanAT May 24, 2022

thomasjpfan May 24, 2022

thomasjpfan May 23, 2022

ShehanAT May 24, 2022

thomasjpfan left a comment

thomasjpfan May 24, 2022

ShehanAT May 24, 2022

thomasjpfan May 24, 2022

ShehanAT May 24, 2022

ShehanAT commented May 24, 2022

thomasjpfan left a comment

cmarmo commented Aug 15, 2022

ShehanAT commented Aug 16, 2022 •

edited

cmarmo commented Aug 16, 2022

ShehanAT commented Aug 18, 2022

cmarmo commented Aug 18, 2022 •

edited

ShehanAT commented Aug 18, 2022

cmarmo commented Aug 19, 2022

lorentzenchr left a comment

	assert result == pytest.approx(0.6666666666666666)
	assert result == pytest.approx(2/3)

		@@ -811,6 +811,7 @@ def check_array(

		if sp.issparse(array):
		_ensure_no_complex_data(array)

		@@ -37,6 +37,7 @@
		from ..preprocessing import label_binarize
		from ..utils._encode import _encode, _unique

	'csr', etc. If the input is sparse but not in the allowed format,
	it will be converted to the first listed format. True allows the input

FIX Enables label_ranking_average_precision_score to support sparse y_true #23442

FIX Enables label_ranking_average_precision_score to support sparse y_true #23442

Conversation

ShehanAT commented May 22, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShehanAT May 23, 2022 • edited

Choose a reason for hiding this comment

thomasjpfan May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShehanAT commented May 24, 2022

thomasjpfan left a comment

Choose a reason for hiding this comment

cmarmo commented Aug 15, 2022

ShehanAT commented Aug 16, 2022 • edited

cmarmo commented Aug 16, 2022

ShehanAT commented Aug 18, 2022

cmarmo commented Aug 18, 2022 • edited

ShehanAT commented Aug 18, 2022

cmarmo commented Aug 19, 2022

lorentzenchr left a comment

Choose a reason for hiding this comment

ShehanAT May 23, 2022 •

edited

thomasjpfan May 23, 2022 •

edited

ShehanAT commented Aug 16, 2022 •

edited

cmarmo commented Aug 18, 2022 •

edited