Sample_weight isn't overwritten anymore in logistic_regression #18480
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rth
reviewed
Sep 28, 2020
| multi_class=multi_class) | ||
| clf.fit(X, y, sample_weight=W) | ||
| actual = W.sum() | ||
| assert expected == actual, 'Sum of weight before ({}) should be the same as sum if weight after ({})'.format(expected, actual) |
There was a problem hiding this comment.
We need to keep lines shorter than 80 char, and also to avoid exact float comparison
Suggested change
| assert expected == actual, 'Sum of weight before ({}) should be the same as sum if weight after ({})'.format(expected, actual) | |
| msg = ( | |
| f'Sum of weight before ({expected}) should be the same as' | |
| f'sum if weight after ({actual})' | |
| ) | |
| assert_allclose(expected, actual, err_msg=msg) |
ghost
commented
Sep 28, 2020
thomasjpfan
reviewed
Sep 29, 2020
Comment on lines
+1870
to
+1883
| @pytest.mark.parametrize("multi_class", {'ovr', 'multinomial', 'auto'}) | ||
| def test_sample_weight_not_modified(multi_class): | ||
| X, y = load_iris(return_X_y=True) | ||
| np.random.seed(1234) | ||
| W = np.random.random(len(X)) * 10.0 | ||
|
|
||
| for weight in [{0: 1.0, 1: 10.0, 2: 1.0}]: | ||
| for class_weight in (weight, 'balanced'): | ||
| expected = W.sum() | ||
|
|
||
| clf = LogisticRegression(random_state=0, | ||
| class_weight=class_weight, | ||
| max_iter=200, | ||
| multi_class=multi_class) |
There was a problem hiding this comment.
We can take advantage of pytest.parametrize:
@pytest.mark.parametrize("multi_class", {'ovr', 'multinomial', 'auto'})
@pytest.mark.parametrize("class_weight", [
{0: 1.0, 1: 10.0, 2: 1.0}, 'balanced'
])
def test_sample_weight_not_modified(multi_class, class_weight):
X, y = load_iris(return_X_y=True)
n_features = len(X)
W = np.ones(n_features)
W[:n_features // 2] = 2
expected = W.sum()
...
glemaitre
reviewed
Oct 7, 2020
| @@ -665,7 +665,7 @@ def _logistic_regression_path(X, y, pos_class=None, Cs=10, fit_intercept=True, | |||
| if isinstance(class_weight, dict) or multi_class == 'multinomial': | |||
| class_weight_ = compute_class_weight(class_weight, | |||
| classes=classes, y=y) | |||
| sample_weight *= class_weight_[le.fit_transform(y)] | |||
| sample_weight = sample_weight * class_weight_[le.fit_transform(y)] | |||
There was a problem hiding this comment.
@rth instead of doing this, would it be better to have copy=False/True within _check_sample_weight signature?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Fixes #18347
Any other comments?