Repository: scikit-learn/scikit-learn
Pull request: [MRG] Standardize sample weights validation in BaseDecisionTree
#### Reference Issues/PRs
Fixes #15358 for BaseDecisionTree

#### What does this implement/fix? Explain your changes.
Replaces custom validation logic with standardized method `utils.validation._check_sample_weight` (relatively newly introduced).

#### Any other comments?
Two things to check:

1. the new validation method only accepts `float32` and `float64`, but the class is using `._tree.DOUBLE` which we think is the same 
2. In the case of an empty array, we now get a TypeError instead of a ValueError due to shared implementation.

We think these are both ok changes.


Problem statement:
Use _check_sample_weight to consistently validate sample_weight
We recently introduced `utils.validation._check_sample_weight` which returns a validated `sample_weight` array.

We should use it consistently throughout the code base, instead of relying on custom and adhoc checks like `check_consistent_lenght` or `check_array` (which are now handled by `_check_sample_weight`).



Here's a list of the estimators/functions that could make use of it (mostly in `fit` or `partial_fit`):

- [x] CalibratedClassifierCV
- [x] DBSCAN
- [x] DummyClassifier
- [x] DummyRegressor
- [x] BaseBagging
- [x] BaseForest
- [x] BaseGradientBoosting
- [x] IsotonicRegression
- [x] KernelRidge
- [x] GaussianNB
- [x] BaseDiscreteNB
- [x] KernelDensity
- [x] BaseDecisionTree

(I left-out the linear_model module because it seems more involved there)

Could be a decent sprint issue @amueller ?

To know where a given class is defined, use e.g. `git grep -n "class DBSCAN"`

Hints from issue discussion:
@NicolasHug I could give it a try. Furthermore, should `_check_sample_weight` also guarantee non-negativeness and sum(sw) > 0 ?
I think for the above mentioned estimators @NicolasHug intended this as an easier refactoring issues for new contributors, but if you want to look into it feel free to open PRs.

> (I left-out the linear_model module because it seems more involved there)

@lorentzenchr  Your expertise would certainly be appreciated there. As you mention https://github.com/scikit-learn/scikit-learn/issues/15438 there is definitely work to be done on improving `sample_weight` handling consistency in linear models. 

> Furthermore, should _check_sample_weight also guarantee non-negativeness and sum(sw) > 0 ?

There are some use cases when it is useful, see https://github.com/scikit-learn/scikit-learn/issues/12464#issuecomment-433815773 but in most cases it would indeed make sense to error on them. In the linked issues it was suggested maybe to enable this check but then allow it to be disabled with a global flag in `sklearn.set_config`. So adding that global config flag and adding the corresponding check in `_check_sample_weight` could be a separate PR.
> intended this as an easier refactoring issues for new contributors

In that case, I will focus on `_check_sample_weight` and on linear models. So new contributors are still welcome 😃 
Thanks @lorentzenchr .

BTW @rth maybe we should add a `return_ones` parameter to `_check_sample_weights`. It would be more convenient than protecting the call with `if sample_weight is not None:...`
Should sample_weights be made an array in all cases? I feel like we have shortcuts if it's ``None`` often and I don't see why we would introduce a multiplication with a constant array.

Also: negative sample weights used to be allowed in tree-based models, not sure if they still are.
> Should sample_weights be made an array in all cases?

No hence my comment above
Oh sorry didn't see that ;)
@fbchow and I will pick up the fix for BaseDecisionTree for the scikitlearn sprint
@fbchow we will do DBSCAN for the wmlds scikitlearn sprint (pair programming @akeshavan)
working on BaseBagging for the wmlds sprint with Honglu Zhang (@ritalulu)

Commit message: Standardize sample weights validation in BaseDecisionTree

Co-authored-by: Sallie Walecka <sallie.walecka@gmail.com>

Patch to review:
diff --git a/sklearn/tree/_classes.py b/sklearn/tree/_classes.py
index c0b1192eafc32..bf78411e2dd1f 100644
--- a/sklearn/tree/_classes.py
+++ b/sklearn/tree/_classes.py
@@ -32,6 +32,7 @@
 from ..utils import Bunch
 from ..utils import check_array
 from ..utils import check_random_state
+from ..utils.validation import _check_sample_weight
 from ..utils import compute_sample_weight
 from ..utils.multiclass import check_classification_targets
 from ..utils.validation import check_is_fitted
@@ -266,18 +267,7 @@ def fit(self, X, y, sample_weight=None, check_input=True,
                               "or larger than 1").format(max_leaf_nodes))
 
         if sample_weight is not None:
-            if (getattr(sample_weight, "dtype", None) != DOUBLE or
-                    not sample_weight.flags.contiguous):
-                sample_weight = np.ascontiguousarray(
-                    sample_weight, dtype=DOUBLE)
-            if len(sample_weight.shape) > 1:
-                raise ValueError("Sample weights array has more "
-                                 "than one dimension: %d" %
-                                 len(sample_weight.shape))
-            if len(sample_weight) != n_samples:
-                raise ValueError("Number of weights=%d does not match "
-                                 "number of samples=%d" %
-                                 (len(sample_weight), n_samples))
+            sample_weight = _check_sample_weight(sample_weight, X, np.float64)
 
         if expanded_class_weight is not None:
             if sample_weight is not None:
diff --git a/sklearn/tree/tests/test_tree.py b/sklearn/tree/tests/test_tree.py
index 4d13f23818e2b..7be38530caab0 100644
--- a/sklearn/tree/tests/test_tree.py
+++ b/sklearn/tree/tests/test_tree.py
@@ -1121,7 +1121,7 @@ def test_sample_weight_invalid():
         clf.fit(X, y, sample_weight=sample_weight)
 
     sample_weight = np.array(0)
-    with pytest.raises(ValueError):
+    with pytest.raises(TypeError):
         clf.fit(X, y, sample_weight=sample_weight)
 
     sample_weight = np.ones(101)


Code review comments:
diff_hunk: @@ -266,18 +267,7 @@ def fit(self, X, y, sample_weight=None, check_input=True,
                               "or larger than 1").format(max_leaf_nodes))
 
         if sample_weight is not None:
-            if (getattr(sample_weight, "dtype", None) != DOUBLE or
-                    not sample_weight.flags.contiguous):
-                sample_weight = np.ascontiguousarray(
-                    sample_weight, dtype=DOUBLE)
-            if len(sample_weight.shape) > 1:
-                raise ValueError("Sample weights array has more "
-                                 "than one dimension: %d" %
-                                 len(sample_weight.shape))
-            if len(sample_weight) != n_samples:
-                raise ValueError("Number of weights=%d does not match "
-                                 "number of samples=%d" %
-                                 (len(sample_weight), n_samples))
+            sample_weight = _check_sample_weight(sample_weight, X, np.float64)
path: sklearn/tree/_classes.py
text: @user1:
Why not keeping `dtype=DOUBLE` ?

@author:
The `_check_sample_weight` only supports `np.float32` and `np.float64`.  

@author:
@user2 

@user1:
`DOUBLE = np.float64`.
While I agree this is equivalent to write `np.float64`, using `DOUBLE` makes it clear that it must be the same type as what is used in the tree cython code. So I would slightly prefer keeping `dtype=DOUBLE`.

@user1:
(this is very much nitpicking)

@user2:
oh, if `DOUBLE` is set to be/mean `np.float64`, then please go with that for consistency and ease of change in the future

@author:
Great! We just updated the method and added a more specific test assertion as per our earlier offline discussion.