ENH add support for sample_weight in KBinsDiscretizer(strategy="quantile") #24935

DeaMariaLeon · 2022-11-15T15:13:33Z

Reference Issues/PRs

Fixes #22048 see also #20758

What does this implement/fix?

Adding support for sample_weight in KBinsDiscretizer(strategy="quantile"). That is to say : I added a parameter in the fit method of KBinsDiscretizer. This parameter, sample_weight can be set to control the weights associated to each sample only when the strategy is set to "quantile"

Additional comments

PR 22048 opened and almost finished by Seladus

…Discretizer

… use of already existing functions.

…ith black

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…acing of conditions to avoid repetitions

… statement and adding a test case for the exception raise on this same check

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…n.py

Merge remote-tracking branch 'upstream/main' into deas-kbins

glemaitre

We should come up with a good test for the sample_weight case.
Somehow, I am thinking that we could come up with an array where we could decide easily the quantile and add some extreme values that could have an impact and pass a corresponding null weight to those samples. We will check in this case that zero-weight will be taken into account in the computation of the quantile.

doc/whats_new/v1.2.rst

sklearn/preprocessing/_discretization.py

glemaitre · 2022-11-15T20:38:49Z

sklearn/preprocessing/_discretization.py

@@ -233,9 +245,28 @@ def fit(self, X, y=None):
                '`subsample` must be used with `strategy="quantile"`.'
            )

+        elif self.strategy != "quantile" and sample_weight is not None:


It seems more logical in this order.

Suggested change

elif self.strategy != "quantile" and sample_weight is not None:

elif sample_weight is not None and self.strategy != "quantile":

sklearn/preprocessing/_discretization.py

sklearn/preprocessing/tests/test_discretization.py

glemaitre · 2022-11-15T20:43:23Z

sklearn/preprocessing/tests/test_discretization.py

@@ -35,6 +50,20 @@ def test_valid_n_bins():
    assert KBinsDiscretizer(n_bins=2).fit(X).n_bins_.dtype == np.dtype(int)


+def test_invalid_sample_weight():


Suggested change

def test_invalid_sample_weight():

@pytest.mark.parametrize("strategy", ["uniform", "kmeans"])

def test_kbinsdiscretizer_wrong_strategy_with_weights(strategy):

glemaitre · 2022-11-15T20:46:03Z

sklearn/preprocessing/tests/test_discretization.py

+    strategy = ["uniform", "kmeans"]
+    sample_weight = [1, 1, 1, 1]
+    for s in strategy:
+        est = KBinsDiscretizer(n_bins=3, strategy=s)
+        err_msg = (
+            "`sample_weight` was provided but it can be only used with"
+            f"strategy='quantile'. Got strategy={s!r} instead."
+        )
+        with pytest.raises(ValueError, match=err_msg):
+            est.fit_transform(X, sample_weight=sample_weight)


Suggested change

strategy = ["uniform", "kmeans"]

sample_weight = [1, 1, 1, 1]

for s in strategy:

est = KBinsDiscretizer(n_bins=3, strategy=s)

err_msg = (

"`sample_weight` was provided but it can be only used with"

f"strategy='quantile'. Got strategy={s!r} instead."

)

with pytest.raises(ValueError, match=err_msg):

est.fit_transform(X, sample_weight=sample_weight)

sample_weight = np.ones(size=(X.shape[0]))

estimator = KBinsDiscretizer(n_bins=3, strategy=strategy)

err_msg = (

"`sample_weight` was provided but it can be only used with"

"strategy='quantile'."

)

with pytest.raises(ValueError, match=err_msg):

est.fit(X, sample_weight=sample_weight)

On line 55 sample_weight = np.ones(size=(X.shape[0])):
X is a list, so we can't use the attribute shape.
Also np.ones has a parameter shape, not size.
Maybe then sample_weight = np.ones(shape=len(X))?

Yes it is indeed the first line of my suggestion :)

sklearn/preprocessing/tests/test_discretization.py

Merge remote-tracking branch 'upstream/main' into deas-kbins

glemaitre

Apart from these small changes. LGTM.

sklearn/preprocessing/tests/test_discretization.py

sklearn/preprocessing/_discretization.py

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Merge remote-tracking branch 'upstream/main' into deas-kbins

DeaMariaLeon · 2022-12-01T10:42:09Z

I did the last changes @glemaitre... Do I need to let somebody know or should I keep waiting for a second reviewer?

I was thinking that I just should wait, but I'm not sure.

Thank you.

glemaitre · 2022-12-01T10:48:11Z

We have to wait for a second reviewer, I will add a tag to inform other core developers that we request a second review. Thanks, @DeaMariaLeon for the work.

DeaMariaLeon · 2022-12-01T10:54:03Z

I should have asked earlier. I was thinking that magically someone was going to see it. 🤓

Thank you @glemaitre for all the teaching. :)

glemaitre · 2022-12-01T10:56:04Z

I should have asked earlier. I was thinking that magically someone was going to see it. 🤓

Indeed, I personally follow-up PR based on email notification that happens only when commenting in PRs (or pushing in it). So if you want to get my attention, you need to explicitly write a comment :)

ogrisel

A few nitpicks and a formatting fix in the error message but other than that, LGTM!

ogrisel · 2022-12-08T18:36:22Z

sklearn/preprocessing/tests/test_discretization.py

+    """Check that we raise an error message when the wrong strategy
+    is used."""


Nitpick PEP 257 style (first line should be a one-liner):

Suggested change

"""Check that we raise an error message when the wrong strategy

is used."""

"""Check that we raise an error when the wrong strategy is used."""

ogrisel · 2022-12-08T18:36:38Z

sklearn/preprocessing/tests/test_discretization.py

+    sample_weight = np.ones(shape=(len(X)))
+    est = KBinsDiscretizer(n_bins=3, strategy=strategy)
+    err_msg = (
+        "`sample_weight` was provided but it can be only used withstrategy='quantile'."


Suggested change

"`sample_weight` was provided but it can be only used withstrategy='quantile'."

"`sample_weight` was provided but it can be only used with "

"strategy='quantile'."

ogrisel · 2022-12-08T18:40:37Z

sklearn/preprocessing/tests/test_discretization.py

@@ -94,6 +146,27 @@ def test_fit_transform_n_bins_array(strategy, expected):
        assert bin_edges.shape == (n_bins + 1,)


+@pytest.mark.filterwarnings("ignore: Bins whose width are too small")
+def test_kbinsdiscretizer_effect_sample_weight():
+    """Check that we take into account `sample_weight` when computing the quantiles."""


Suggested change

"""Check that we take into account `sample_weight` when computing the quantiles."""

"""Check the impact of `sample_weight` one computed quantiles."""

ogrisel · 2022-12-08T18:42:57Z

sklearn/preprocessing/tests/test_discretization.py

+
+
+def test_kbinsdiscretizer_no_mutating_sample_weight():
+    """Check to make sure that `sample_weight` is no changed in place."""


Suggested change

"""Check to make sure that `sample_weight` is no changed in place."""

"""Make sure that `sample_weight` is not changed in place."""

sklearn/preprocessing/tests/test_discretization.py

ogrisel · 2022-12-08T18:51:25Z

sklearn/preprocessing/_discretization.py

+                "`sample_weight` was provided but it can be only used with"
+                f"strategy='quantile'. Got strategy={self.strategy!r} instead."


Suggested change

"`sample_weight` was provided but it can be only used with"

f"strategy='quantile'. Got strategy={self.strategy!r} instead."

"`sample_weight` was provided but it can only be "

f"used with strategy='quantile'. Got strategy="

f"{self.strategy!r} instead."

Merge remote-tracking branch 'upstream/main' into deas-kbins

DeaMariaLeon · 2022-12-09T18:00:03Z

Implemented latest corrections. It should be completed.

glemaitre

Since we released 1.2 last week, we need to update the changelog for this feature. Otherwise, still LGTM.

glemaitre · 2022-12-12T13:36:17Z

doc/whats_new/v1.2.rst

@@ -680,6 +680,13 @@ Changelog
  is now deprecated and will be removed in version 1.4. Use `sparse_output` instead.
  :pr:`24412` by :user:`Rushil Desai <rusdes>`.

+- |Enhancement| Added support for `sample_weight` in :class:`preprocessing.KBinsDiscretizer`.


We just released 1.2. We will need to move this section to the v1.3.rst file

glemaitre · 2022-12-12T13:36:24Z

sklearn/preprocessing/_discretization.py

+            Contains weight values to be associated with each sample.
+            Only possible when `strategy` is set to `"quantile"`.
+
+            .. versionadded:: 1.2


Suggested change

.. versionadded:: 1.2

.. versionadded:: 1.3

Merge remote-tracking branch 'upstream/main' into deas-kbins

DeaMariaLeon · 2022-12-12T17:36:53Z

Changed release news files.

However... There is this note in the instructions for documentation contributors that I just saw:
"When editing reStructuredText (.rst) files, try to keep line length under 80 characters when possible (exceptions include links and tables)."
I tried to keep it under 80 characters but it looked weird.

Also: "Before submitting your pull request check if your modifications have introduced new sphinx warnings and try to fix them."
When or where should I see the warnings?

Thanks

glemaitre · 2022-12-12T18:05:39Z

doc/whats_new/v1.3.rst

+- |Enhancement| Added support for `sample_weight` in :class:`preprocessing.KBinsDiscretizer`.
+  This allows specifying the parameter `sample_weight` for each sample to be used while
+  fitting. The option is only available when `strategy` is set to `quantile`.
+  :pr:`24935` by :user:`Seladus <seladus>`,
+  :user:`Guillaume Lemaitre <glemaitre>`, and
+  :user:`Dea María Léon <deamarialeon>`.


This would be less than 80 characters. This would be fine.

Suggested change

- |Enhancement| Added support for `sample_weight` in :class:`preprocessing.KBinsDiscretizer`.

This allows specifying the parameter `sample_weight` for each sample to be used while

fitting. The option is only available when `strategy` is set to `quantile`.

:pr:`24935` by :user:`Seladus <seladus>`,

:user:`Guillaume Lemaitre <glemaitre>`, and

:user:`Dea María Léon <deamarialeon>`.

- |Enhancement| Added support for `sample_weight` in

:class:`preprocessing.KBinsDiscretizer`. This allows specifying the parameter

`sample_weight` for each sample to be used while fitting. The option is only

available when `strategy` is set to `quantile`.

:pr:`24935` by :user:`Seladus <seladus>`, :user:`Guillaume Lemaitre <glemaitre>`, and

:user:`Dea María Léon <deamarialeon>`.

DeaMariaLeon · 2022-12-12T18:34:57Z

Done.. I think.

glemaitre · 2022-12-13T16:11:56Z

All good. Thanks @DeaMariaLeon

DeaMariaLeon · 2022-12-13T16:27:18Z

Thank you @glemaitre

…quantile" (scikit-learn#24935) Co-authored-by: seladus <clement.blancovolle@insa-rouen.fr> Co-authored-by: Seladus <71873495+Seladus@users.noreply.github.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Seladus and others added 24 commits December 21, 2021 13:58

Adding sample_weight parameter support in fit method of KBinsDiscretizer

3522d2d

Adding support for sample_weights in the case of an array-like n_bins

4366bdc

Adding test cases for sample_weights parameter in fit method in KBins…

17e429b

…Discretizer

Black formatting and clarifications in documentation

fe6076d

Minor fix for PEP8 compatibility

e7d5003

Adding fix to correct misunderstanding of the task and to make better…

85fe2b7

… use of already existing functions.

Adding parameter copy=True in check for sample_weights + formatting w…

83a0bba

…ith black

Removing unused imports

524cb73

Adding entry to the changelog

dbe2eb0

Adding dtype in bin edges construction

6bff3a3

Update sklearn/preprocessing/_discretization.py

ea5f446

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Application of suggestions : sooner check for valid strategy + interl…

e7e9eee

…acing of conditions to avoid repetitions

Movins subsample check for other strategy than quantile in its own if…

e78c263

… statement and adding a test case for the exception raise on this same check

Change for linter

1a417f4

Merge branch 'main' into support_sample_weight_in_kbinsdiscretizer

b5677e3

Update sklearn/preprocessing/_discretization.py

b16321c

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update _discretization.py

0d3919b

Adding TODO comment in tests and removing uselesss call to np.array

30edabc

Changes for linter

b600a0d

Merge branch 'main' into deas-kbins

80f9247

Revert pre-commit file change

5c573bd

removed test_invalid_n_bins() & trailing spaces to test_discretizatio…

9498a89

…n.py

Finishing PR 22048 - updating my branch with upstream

7ab4801

Merge remote-tracking branch 'upstream/main' into deas-kbins

Finishing PR 22048

1736672

Merge remote-tracking branch 'upstream/main' into deas-kbins

github-actions bot added the module:preprocessing label Nov 15, 2022

glemaitre reviewed Nov 15, 2022

View reviewed changes

DeaMariaLeon added 4 commits November 21, 2022 19:50

implemented suggestions before adding the new tests

0cacab4

added tests for sample_weight

1957a1f

Merging upstream/main

a48b3aa

Merge remote-tracking branch 'upstream/main' into deas-kbins

corrected errors w/black

ff565e5

merge upstream/main

bc10682

Merge remote-tracking branch 'upstream/main' into deas-kbins

glemaitre self-requested a review November 23, 2022 16:20

glemaitre approved these changes Nov 23, 2022

View reviewed changes

DeaMariaLeon and others added 3 commits November 24, 2022 19:54

Apply suggestions from code review

f799b33

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Applied remaining suggestions

fd34d50

marge upstream

1d334ca

Merge remote-tracking branch 'upstream/main' into deas-kbins

glemaitre added the Waiting for Second Reviewer First reviewer is done, need a second one! label Dec 1, 2022

ogrisel approved these changes Dec 8, 2022

View reviewed changes

DeaMariaLeon added 2 commits December 8, 2022 21:36

Corrected to comply w/PEP 257

702168a

"merging upstream/main"

1dbbbcf

Merge remote-tracking branch 'upstream/main' into deas-kbins

glemaitre approved these changes Dec 12, 2022

View reviewed changes

DeaMariaLeon added 2 commits December 12, 2022 18:23

Moved enhancement note from v1.2.rst to v1.3.rst

822bd09

Merge upstream

a1dcd58

Merge remote-tracking branch 'upstream/main' into deas-kbins

glemaitre reviewed Dec 12, 2022

View reviewed changes

Enhancement note on v1.3.rst to less than 80 characters

cd519ef

glemaitre merged commit 1f3c1be into scikit-learn:main Dec 13, 2022

DeaMariaLeon deleted the deas-kbins branch December 13, 2022 16:27

lorentzenchr mentioned this pull request Dec 18, 2022

Support sample_weight in KBinsDiscretizer(strategy="quantile") #20758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH add support for sample_weight in KBinsDiscretizer(strategy="quantile") #24935

ENH add support for sample_weight in KBinsDiscretizer(strategy="quantile") #24935

DeaMariaLeon commented Nov 15, 2022

glemaitre left a comment

glemaitre Nov 15, 2022

glemaitre Nov 15, 2022

glemaitre Nov 15, 2022

DeaMariaLeon Nov 21, 2022

glemaitre Nov 23, 2022

glemaitre left a comment

DeaMariaLeon commented Dec 1, 2022

glemaitre commented Dec 1, 2022

DeaMariaLeon commented Dec 1, 2022

glemaitre commented Dec 1, 2022

ogrisel left a comment

ogrisel Dec 8, 2022

ogrisel Dec 8, 2022

ogrisel Dec 8, 2022

ogrisel Dec 8, 2022

ogrisel Dec 8, 2022

DeaMariaLeon commented Dec 9, 2022

glemaitre left a comment

glemaitre Dec 12, 2022

glemaitre Dec 12, 2022

DeaMariaLeon commented Dec 12, 2022

glemaitre Dec 12, 2022

DeaMariaLeon commented Dec 12, 2022

glemaitre commented Dec 13, 2022

DeaMariaLeon commented Dec 13, 2022

	elif self.strategy != "quantile" and sample_weight is not None:
	elif sample_weight is not None and self.strategy != "quantile":

		@@ -35,6 +50,20 @@ def test_valid_n_bins():
		assert KBinsDiscretizer(n_bins=2).fit(X).n_bins_.dtype == np.dtype(int)


		def test_invalid_sample_weight():

	def test_invalid_sample_weight():
	@pytest.mark.parametrize("strategy", ["uniform", "kmeans"])
	def test_kbinsdiscretizer_wrong_strategy_with_weights(strategy):

		"""Check that we raise an error message when the wrong strategy
		is used."""

	"""Check that we raise an error message when the wrong strategy
	is used."""
	"""Check that we raise an error when the wrong strategy is used."""

	"`sample_weight` was provided but it can be only used withstrategy='quantile'."
	"`sample_weight` was provided but it can be only used with "
	"strategy='quantile'."

	"""Check that we take into account `sample_weight` when computing the quantiles."""
	"""Check the impact of `sample_weight` one computed quantiles."""



		def test_kbinsdiscretizer_no_mutating_sample_weight():
		"""Check to make sure that `sample_weight` is no changed in place."""

	"""Check to make sure that `sample_weight` is no changed in place."""
	"""Make sure that `sample_weight` is not changed in place."""

		"`sample_weight` was provided but it can be only used with"
		f"strategy='quantile'. Got strategy={self.strategy!r} instead."

-                "`sample_weight` was provided but it can be only used with"
-                f"strategy='quantile'. Got strategy={self.strategy!r} instead."
+                "`sample_weight` was provided but it can only be "
+                f"used with strategy='quantile'. Got strategy="
+                f"{self.strategy!r} instead."

ENH add support for sample_weight in KBinsDiscretizer(strategy="quantile") #24935

ENH add support for sample_weight in KBinsDiscretizer(strategy="quantile") #24935

Conversation

DeaMariaLeon commented Nov 15, 2022

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

DeaMariaLeon commented Dec 1, 2022

glemaitre commented Dec 1, 2022

DeaMariaLeon commented Dec 1, 2022

glemaitre commented Dec 1, 2022

ogrisel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeaMariaLeon commented Dec 9, 2022

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeaMariaLeon commented Dec 12, 2022

Choose a reason for hiding this comment

DeaMariaLeon commented Dec 12, 2022

glemaitre commented Dec 13, 2022

DeaMariaLeon commented Dec 13, 2022