Skip to content

ENH use utility _check_sample_weight in IsotonicRegression#16322

Merged
ogrisel merged 4 commits intoscikit-learn:masterfrom
Batalex:refact/check-isotonic
Jan 30, 2020
Merged

ENH use utility _check_sample_weight in IsotonicRegression#16322
ogrisel merged 4 commits intoscikit-learn:masterfrom
Batalex:refact/check-isotonic

Conversation

@Batalex
Copy link
Copy Markdown
Contributor

@Batalex Batalex commented Jan 30, 2020

Reference Issues/PRs

Continuation of #16203 so that we can close #15358.

What does this implement/fix? Explain your changes.

This PR's addition is to reorder the sample_weights array as discussed in the original PR.
I force the array to be contiguous otherwise a test would fail locally for this very reason.

Any other comments?

2020 Paris Sprint

Copy link
Copy Markdown
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Nit but LGTM, thanks @Batalex

Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. @NicolasHug are you fine with merging without your suggestion?

@NicolasHug
Copy link
Copy Markdown
Member

yes

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Jan 30, 2020

I saw #16322 (comment) just after my comment above.

As you wish. +0 for the None weight fast path option.

@jeremiedbb
Copy link
Copy Markdown
Member

As you wish. +0 for the None weight fast path option.

let's ignore that for this PR

@ogrisel ogrisel merged commit 5ea6a26 into scikit-learn:master Jan 30, 2020
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Jan 30, 2020

Merged then! Thank you very much @Batalex.

@Batalex Batalex deleted the refact/check-isotonic branch January 30, 2020 16:21
@NicolasHug
Copy link
Copy Markdown
Member

+0 for the None weight fast path option.

It's not applicable here anyway because the fast way isn't implemented for IsotonicRegeression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use _check_sample_weight to consistently validate sample_weight

5 participants