I am trying to figure out how to handle the case where the input y to transform() or label_binarize() is a sparse matrix. This might call for extending sparse support for utils.multiclass.type_of_target() since it currently fails on sparse matrices.

Supporting sparse in type_of_target is definitely something that's needed,
and is present in Rohit's PR (and was copied into my sparse metrics PR).
However, it's altogether not clear that LabelBinarizer should accept
binarized input in the first place, but there are arguments to do so.

On 28 May 2014 07:57, hamsal notifications@github.com wrote:

I am trying to figure out how to handle the case where the input y to
transform() or label_binarize() is a sparse matrix. This might call for
extending sparse support for utils.multiclass.type_of_target() since it
currently fails on sparse matrices.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3203#issuecomment-44340966
.

Sparse support for type_of_target has been included based on the comments in Rohit's PR. The support for sparse input as it is right now will really only serve to validate the input sparse matrix as being correctly constructed there is no conversion from any format that is not multilabel-indicator to multilabel-indicator. This does seem more correct then failing on the input when it is a sparse multilabel-indicator matrix.

I am getting some numerical precision issues in the doctests that I think may be unrelated to the changes of this PR. Advice on how to proceed would be appreciated. I am running on a 64 bit machine but the numbers calculated contain fewer significant figures.

If i understand correctly the failure, a piece od code uses the label binarizer with the multilabel argument. But you seprecatr this parameter. I would simply correct the function call in the appropriate place.

The multilabel error has been fixed, the precision errors have come back on the Travis CI.

There is probably a change in the dtype which cause the small difference.
If you don't find the cause, you can add the ellipsis directive in the docstring.

Coverage decreased (-5.08%) when pulling b4db6f2 on hamsal:sprs-lbl-bin into 1e64b7b on scikit-learn:master.

Thanks for the point to the ellipsis directive! The sparse parameter has been renamed sparse_output. The unnecessary helper function was removed, and dependencies on self.multilabel_ were removed.

The test coverage needs to be improved.

Hm apparently you deleted and added some files.

You forgot the ellipsis ... :-)

Coverage decreased (-5.08%) when pulling b4db6f2 on hamsal:sprs-lbl-bin into 1e64b7b on scikit-learn:master.

Coverage decreased (-2.66%) when pulling 69cd58e on hamsal:sprs-lbl-bin into 1e64b7b on scikit-learn:master.

To undo properly the file deletion, you can use:

$ git filter-branch --force --index-filter 'git checkout master path/to/file' --prune-empty master..this-branch

But use with care, it rewrite the history of the branch.

I don't understand how you get -2.66% of decreased coverage.

I did not change code in my last commit and yet coverage changed significantly which is curious.

About the files, they were not deletions they were changes of the access mode in the OS. I accidentally did git add on those four files so the access mode change appeared in the differential, I undid the changes to the files and I believe the situation is OK now.

Coverage decreased (-2.66%) when pulling c99f1b9 on hamsal:sprs-lbl-bin into 1e64b7b on scikit-learn:master.

Apparently, you have merge conflicts.

Looks good to me, switch the title to MRG+1

Yes, we I think I am also +1 for merge. Thank you @hamsal.

One small comment: there are a few tests, in this PR and elsewhere, that iterate through a number of sparse formats. In a new PR after this one is merged, could we please make this list a constant in sklearn.utils.testing? Alternatively, it may be a better idea to just use one format (e.g. CSR) and then exploit the .asformat method of sparse matrices, as (almost) in sklearn/decomposition/tests/test_truncated_svd.py. In any case, the set of supported matrix formats should be centralised.

One small comment: there are a few tests, in this PR and elsewhere, that iterate through a number of sparse formats. In a new PR after this one is merged, could we please make this list a constant in sklearn.utils.testing? Alternatively, it may be a better idea to just use one format (e.g. CSR) and then exploit the .asformat method of sparse matrices, as (almost) in sklearn/decomposition/tests/test_truncated_svd.py. In any case, the set of supported matrix formats should be centralised.

+1 for your proposition @jnothman. Can you open an issue or make a pull request?
edit (I mean for the list of sparse format)

merging

merged as 2bfca14

In my git rebases and history edits it looks like there are 3 commits in this PR that just came into the PR after the most recent rebase. So I don't think this was ready for a merge! Please look at the commit history and see that there are three commits shared by @jnothman and @arjoly.

Edit: These commits apparently also introduced the two new files in the files changed that have to do with the tree classifier.

I should have made it clearer that I was also attempting to fix errors, I do not think the most recent Travis passed.

Oh! Whoops... What do you mean by there being commits shared by jnothman and arjoly?

You are right a test fails. Could you please submit a patch?

The commit history has three commits, two from you and one from Arnaud.

Arnaud Joly arjoly MAINT reduce amount of boiler code using standard C operations bd7db7b

jnothman jnothman DOC give example of binarizing binary targets … eec979c

jnothman jnothman DOC Note shape of binary binarized output 34872e9

Yes I can submit a patch for the Travis error, I am not sure what to do about these stary commits or if they are a problem.

don't worry about that. it's squashed to one commit.

Ok good to know

But the test error makes it seem like the label binarizer is outputting non-integer labels. That should probably be locally tested rather than triggering an error in test_multiclass.py

I'm very sorry to have not properly tested before pushing, so hopefully we can fix this quickly.

This error has been confusing me because I can not recreate it locally, I have rebased and I have verified that my numpy is over 1.6.2 So for now the only place I have seen it is on Travis.

Edit: I have recreated it locally

I am working on the patch here #3314

I have fixed the error locally, Travis is building it now.

The patch has been merged, I apologize for the headache and thank you for the reviews!

Thanks for the good work! 🍻