Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOC Expand multilabel in decision function in glossary #24095

Merged
merged 4 commits into from Aug 24, 2022

Conversation

lucyleeow
Copy link
Member

@lucyleeow lucyleeow commented Aug 3, 2022

Reference Issues/PRs

(Continues from stalled PR) Closes #13660
closes #13533.

What does this implement/fix? Explain your changes.

Expand multilabel in decision function in glossary, using the suggestion: #13660 (comment)

I didn't include the code from the gist as no other section of the glossary included code. I could give an example based off of the code in the gist (e.g., dataset of 5 labels and 3 samples, the shape would be: ... ) but was not sure. Happy to take suggestions

Any other comments?

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. It's interesting how the glossary goes straight to the decision function. When I introduce multi-label classification, I usually go to the target format first: https://scikit-learn.org/stable/modules/multiclass.html#multiclass-and-multioutput-algorithms

Overall, I think using a list makes the glossary entry clearer, so I am +1.

interpreted, like in the binary case, by thresholding at 0.

TODO: `This gist
<https://gist.github.com/jnothman/4807b1b0266613c20ba4d1f88d0f8cf5>`_
Copy link
Member

Choose a reason for hiding this comment

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

I think it is unclear what the TODO is here. @jnothman What did you have in mind?

@lucyleeow
Copy link
Member Author

lucyleeow commented Aug 4, 2022

when I introduce multi-label classification, I usually go to the target format first

Fair point, looking more closely I see that there is a target types section above: https://scikit-learn.org/stable/glossary.html#target-types - I've added a link to the glossary term 'multilabel'

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

Thanks @lucyleeow !

doc/glossary.rst Outdated

- Single 2d array of shape (`n_samples`, `n_labels`), with each
'column' in the array corresponding to the individual binary
classification decisions. This is ambiguously identical to the
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Is "ambiguously" necessary?

@glemaitre glemaitre merged commit c8bef4b into scikit-learn:main Aug 24, 2022
@glemaitre
Copy link
Member

LGTM Thanks @lucyleeow

@lucyleeow lucyleeow deleted the doc_multi branch August 25, 2022 00:05
@lucyleeow
Copy link
Member Author

Thanks all!

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TODO comment found in Glossary (Documentation) in Data Type section
4 participants