[MRG] Adding variable force_alpha to classes in naive_bayes.py #18805

hongshaoyang · 2020-11-10T09:22:33Z

Reference Issues/PRs

Fixes #10772
References PR #9131
References PR #10775
Taking over stalled PR #16747

What does this implement/fix? Explain your changes.

This PR takes over stalled PR #16747.

From the description of #16747: "This PR adds a new variable alphaCorrection in classes in naive_bayes.py, which is set to True by default and if set to False, then for alpha=0 (or greater, but still smaller than _ALPHA_MIN) alpha is not being rounded up to _ALPHA_MIN."

I merged master and fixed conflicts, no other changes.

Any other comments?

#16747 (comment) should be resolved.

Merging changes from the main repository

Update branch

…-update Resolving conflicts

…ka204/scikit-learn into alpha-close-or-equal-0-update

…-update Alpha close or equal 0 update

Update master

…a-is-close-or-equal-0' into master-copy

Update branch

…ernoulliNB-and-MultinomialNB-when-alpha-is-close-or-equal-0 # Conflicts: # doc/whats_new/v0.24.rst # sklearn/naive_bayes.py

hongshaoyang · 2020-11-20T14:28:53Z

Gentle ping @cmarmo @jnothman @amueller

cmarmo · 2020-12-18T15:59:03Z

Hi @hongshaoyang , sorry your pull request didn't make it in 0.24. Do you mind moving your what's new entry from v0.24.rst to v1.0.rst? Thanks!

# Conflicts: # doc/whats_new/v1.0.rst

jjerphan

I am catching up with this PR.
I just have one last suggestion prior to the merge.

sklearn/naive_bayes.py

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan · 2021-06-09T11:41:50Z

Small tip: I would suggest to both make modifications and run tests locally prior to pushing new commits.

This would allow you better take suggestions into account, group suggestions (if relevant) and assert if you've missed something (as suggestions sometimes necessitate other patches than theirs) without going back and forth with the CI logs. 🙂

hongshaoyang · 2021-06-09T11:46:32Z

Thank you! I was eager to commit the suggestions without taking tests into account. Your suggestion is helpful!

jjerphan · 2021-06-23T08:24:01Z

Hi @hongshaoyang,

In the meantime, black code formatting was set on main, causing conflict on this branch.

Instructions given in #20301 (comment) might help you with resolving them.

Let's then merge it once resolved. 🙂

…rce-alpha

Following scikit-learn#20301 (comment)

# Conflicts: # sklearn/naive_bayes.py

jjerphan

LGTM @hongshaoyang. 👍

I just have one last suggestion.

sklearn/tests/test_naive_bayes.py

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

cmarmo · 2021-11-12T21:42:07Z

Hi @hongshaoyang, and thanks for your patience. If you are still motivated, this PR could be a nice fix for version 1.1.
Do you mind fixing conflicts and updating the version number to 1.1? Then let's see if perhaps @TomDLT might be available for a quick review? Thanks!

TomDLT

Some comments.

Main question: Do we really need to warn when the user explicitly set force_alpha=True ?

TomDLT · 2021-11-12T21:48:29Z

doc/whats_new/v1.0.rst

@@ -462,6 +462,11 @@ Changelog

 :mod:`sklearn.naive_bayes`
 ..........................
+- |Fix| A new parameter `force_alpha` was added to :class:`BernoulliNB` and
+  class:`MultinomialNB`, allowing user to set parameter alpha to a very


Suggested change

class:`MultinomialNB`, allowing user to set parameter alpha to a very

:class:`MultinomialNB`, allowing user to set parameter alpha to a very

Also need to mention ComplementNB and CategoricalNB.

TomDLT · 2021-11-12T21:49:41Z

doc/whats_new/v1.0.rst

+- |Fix| A new parameter `force_alpha` was added to :class:`BernoulliNB` and
+  class:`MultinomialNB`, allowing user to set parameter alpha to a very
+  small number, greater or equal 0, which was earlier automatically changed
+  to `_ALPHA_MIN` instead.


I would change _ALPHA_MIN into its value to be more informative.

TomDLT · 2021-11-12T21:50:58Z

sklearn/naive_bayes.py

+
+    force_alpha : bool, default=False
+        If False and alpha is too close to 0, it will set alpha to
+        `_ALPHA_MIN`. If True, warn user about potential numeric errors


I would change _ALPHA_MIN into its value to be more informative.

TomDLT · 2021-11-12T21:51:23Z

sklearn/naive_bayes.py

+        `_ALPHA_MIN`. If True, warn user about potential numeric errors
+        and proceed with alpha unchanged.
+
+        .. versionadded:: 1.0


Suggested change

.. versionadded:: 1.0

.. versionadded:: 1.1

TomDLT · 2021-11-12T21:52:00Z

doc/whats_new/v1.0.rst

@@ -462,6 +462,11 @@ Changelog

 :mod:`sklearn.naive_bayes`
 ..........................
+- |Fix| A new parameter `force_alpha` was added to :class:`BernoulliNB` and


Because v1.0 is already released, this entry should now be moved to v1.1.rst.

TomDLT · 2021-11-12T21:55:14Z

sklearn/naive_bayes.py

-            )
-            return np.maximum(self.alpha, _ALPHA_MIN)
+            if self.force_alpha:
+                warnings.warn(


Is this really useful to warn when the user specifically set the parameter force_alpha=True ? I would remove the warning and improve the docstring to mention potential numerical errors.

TomDLT · 2021-11-12T21:55:36Z

sklearn/naive_bayes.py

+        `_ALPHA_MIN`. If True, warn user about potential numeric errors
+        and proceed with alpha unchanged.
+
+        .. versionadded:: 1.0


Suggested change

.. versionadded:: 1.0

.. versionadded:: 1.1

TomDLT · 2021-11-12T21:55:49Z

sklearn/naive_bayes.py

+        `_ALPHA_MIN`. If True, warn user about potential numeric errors
+        and proceed with alpha unchanged.
+
+        .. versionadded:: 1.0


Suggested change

.. versionadded:: 1.0

.. versionadded:: 1.1

TomDLT · 2021-11-12T21:56:01Z

sklearn/naive_bayes.py

+        `_ALPHA_MIN`. If True, warn user about potential numeric errors
+        and proceed with alpha unchanged.
+
+        .. versionadded:: 1.0


Suggested change

.. versionadded:: 1.0

.. versionadded:: 1.1

TomDLT · 2021-11-12T22:05:21Z

sklearn/naive_bayes.py

+                )
+            else:
+                warnings.warn(
+                    "alpha too small will result in numeric errors, "


I would add something like "use force_alpha=True to keep alpha unchanged.".

Micky774 · 2022-01-22T19:12:31Z

Picked up in PR #22269

cmarmo · 2022-07-09T02:30:15Z

Closing this as superseded by #22269 .

arka204 and others added 18 commits March 22, 2020 11:06

Adding variable alphaCorrection to classes in naive_bayes.py.

282a7dd

Splitting few lines of code.

d78e17b

Merge pull request scikit-learn#1 from scikit-learn/master

3b79637

Merging changes from the main repository

Merge pull request scikit-learn#2 from scikit-learn/master

464dc37

Merging changes from the main repository

Fixing problems and adding tests.

a4429bf

Updating naive_bayes.py.

cf35eb1

Merge pull request scikit-learn#3 from scikit-learn/master

15a658f

Merging changes from the main repository

Merge branch 'alpha-close-or-equal-0-update' into alpha-1

ec786b3

Merge pull request scikit-learn#5 from arka204/alpha-1

4606d85

Update branch

Merge pull request scikit-learn#6 from arka204/alpha-close-or-equal-0…

f0debb6

…-update Resolving conflicts

Checkig warnings in tests.

dcce4a8

Merge branch 'alpha-close-or-equal-0-update' of https://github.com/ar…

81d5f32

…ka204/scikit-learn into alpha-close-or-equal-0-update

Merge pull request scikit-learn#8 from arka204/alpha-close-or-equal-0…

43dfda5

…-update Alpha close or equal 0 update

Update v0.24.rst

be0ebfe

Merge pull request scikit-learn#10 from scikit-learn/master

2968400

Update master

Merge branch 'Proposition-for-BernoulliNB-and-MultinomialNB-when-alph…

7e1b649

…a-is-close-or-equal-0' into master-copy

Merge pull request scikit-learn#11 from arka204/master-copy

5782e6f

Update branch

Merge remote-tracking branch 'upstream/master' into Proposition-for-B…

a7337d2

…ernoulliNB-and-MultinomialNB-when-alpha-is-close-or-equal-0 # Conflicts: # doc/whats_new/v0.24.rst # sklearn/naive_bayes.py

github-actions bot added the module:naive_bayes label Nov 10, 2020

hongshaoyang force-pushed the 10772-force-alpha branch from 4b1802a to 06f6779 Compare November 10, 2020 09:25

Fix merge

2d16091

hongshaoyang force-pushed the 10772-force-alpha branch from 06f6779 to 2d16091 Compare November 10, 2020 09:28

hongshaoyang mentioned this pull request Nov 10, 2020

[MRG] Adding variable alphaCorrection to classes in naive_bayes.py. #16747

Closed

cmarmo added the Waiting for Reviewer label Dec 14, 2020

hongshaoyang added 3 commits December 20, 2020 16:56

Merge remote-tracking branch 'upstream/master' into 10772-force-alpha

728b842

Move whatsnew

a8209e0

Merge remote-tracking branch 'upstream/master' into 10772-force-alpha

44c50fd

# Conflicts: # doc/whats_new/v1.0.rst

Base automatically changed from master to main January 22, 2021 10:53

Fix merge

1d01c6c

hongshaoyang changed the title ~~[MRG] Adding variable alphaCorrection to classes in naive_bayes.py~~ [MRG] Adding variable force_alpha to classes in naive_bayes.py May 29, 2021

use assert_warns_message

aa1d8de

jjerphan requested changes Jun 8, 2021

View reviewed changes

sklearn/naive_bayes.py Outdated Show resolved Hide resolved

hongshaoyang and others added 4 commits June 9, 2021 16:23

8000

Apply suggestions from code review

203af9e

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Merge remote-tracking branch 'upstream/main' into 10772-force-alpha

cc4fda7

Fix wrong variable name

91127bc

Fix test to use "with pytest.warns" instead of assert_warns_message

c4d0736

hongshaoyang and others added 4 commits June 23, 2021 18:01

Merge commit '0e7761cdc4f244adb4803f1a97f0a9fe4b365a99' into 10772-fo…

8964a16

…rce-alpha

MAINT Adds target_version to black config (scikit-learn#20293)

e7a5f37

Black formatting

98c0c12

Following scikit-learn#20301 (comment)

Merge remote-tracking branch 'upstream/main' into 10772-force-alpha

2d9ab41

# Conflicts: # sklearn/naive_bayes.py

jjerphan approved these changes Jun 23, 2021

View reviewed changes

sklearn/tests/test_naive_bayes.py Show resolved Hide resolved

Apply suggestions from code review

16af708

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

TomDLT reviewed Nov 12, 2021

View reviewed changes

cmarmo removed the Waiting for Reviewer label Nov 12, 2021

cmarmo added help wanted Stalled labels Jan 15, 2022

Micky774 mentioned this pull request Jan 22, 2022

ENH Adding variable force_alpha to classes in naive_bayes.py #22269

Merged

cmarmo added Superseded PR has been replace by a newer PR and removed Stalled help wanted labels Jan 22, 2022

cmarmo closed this Jul 9, 2022

hongshaoyang deleted the 10772-force-alpha branch July 30, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Adding variable force_alpha to classes in naive_bayes.py #18805

[MRG] Adding variable force_alpha to classes in naive_bayes.py #18805

	class:`MultinomialNB`, allowing user to set parameter alpha to a very
	:class:`MultinomialNB`, allowing user to set parameter alpha to a very

[MRG] Adding variable force_alpha to classes in naive_bayes.py #18805

[MRG] Adding variable force_alpha to classes in naive_bayes.py #18805

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment