[MRG+1] Fix MultinomialNB and BernoulliNB alpha=0 bug (continuation)#9131
Merged
jmschrei merged 17 commits intoscikit-learn:masterfrom Jun 19, 2017
Merged
[MRG+1] Fix MultinomialNB and BernoulliNB alpha=0 bug (continuation)#9131jmschrei merged 17 commits intoscikit-learn:masterfrom
jmschrei merged 17 commits intoscikit-learn:masterfrom
Conversation
…into fix_NB_5814
jnothman
reviewed
Jun 15, 2017
Member
jnothman
left a comment
There was a problem hiding this comment.
Thanks. Please wrap those fit calls in assert_warns in tests to ensure the warning is triggered.
We should also have a small test for the ValueError case, using assert_raise_messsage or similar.
Thanks for taking this on.
Contributor
Author
|
Thanks for reviewing! Tests are done. |
jnothman
reviewed
Jun 15, 2017
sklearn/tests/test_naive_bayes.py
Outdated
| # Test sparse X | ||
| X = scipy.sparse.csr_matrix(X) | ||
| nb = BernoulliNB(alpha=0.) | ||
| nb.fit(X, y) |
Member
There was a problem hiding this comment.
This will also raise the warning which we would rather not see in tests. Either assert or ignore.
sklearn/tests/test_naive_bayes.py
Outdated
| y = np.array([0, 1]) | ||
| b_nb = BernoulliNB(alpha=-0.1) | ||
| m_nb = MultinomialNB(alpha=-0.1) | ||
| assert_raises(ValueError, b_nb.fit, X, y) |
Member
There was a problem hiding this comment.
Better if this tests the error message with the assert_raise_* variants
Member
|
Please add an entry to the change log at |
jnothman
reviewed
Jun 15, 2017
sklearn/naive_bayes.py
Outdated
|
|
||
| def _check_alpha(self): | ||
| if self.alpha < 0: | ||
| raise ValueError('Smoothing parameter alpha = %e. ' |
Member
There was a problem hiding this comment.
We can now see that %e is clearly a bad pick. %.1e would be better and just as informative
Contributor
Author
There was a problem hiding this comment.
Very good point! done
jnothman
approved these changes
Jun 16, 2017
Member
jnothman
left a comment
There was a problem hiding this comment.
Otherwise, and assuming tests pass, LGTM
sklearn/naive_bayes.py
Outdated
| def _check_alpha(self): | ||
| if self.alpha < 0: | ||
| raise ValueError('Smoothing parameter alpha = %.1e. ' | ||
| 'alpha must be >= 0!' % self.alpha) |
Member
There was a problem hiding this comment.
Just noticed this says >= which is a little awkward we warn when it's 0 and change the value
Member
There was a problem hiding this comment.
Perhaps alternate language is "alpha should be > 0", as opposed to 'must be'?
Contributor
Author
There was a problem hiding this comment.
thanks, it's already fixed
jmschrei
reviewed
Jun 16, 2017
| else: | ||
| self.class_log_prior_ = np.zeros(n_classes) - np.log(n_classes) | ||
|
|
||
| def _check_alpha(self): |
Member
There was a problem hiding this comment.
Maybe you went through this before, but why isn't alpha just initially set to an appropriate value and then used as before, instead of changing the code a lot as below?
Member
|
Modifying attributes didn't pay nice with our set_params API
…On 16 Jun 2017 10:36 am, "Jacob Schreiber" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In sklearn/naive_bayes.py
<#9131 (comment)>
:
> @@ -460,6 +463,16 @@ def _update_class_log_prior(self, class_prior=None):
else:
self.class_log_prior_ = np.zeros(n_classes) - np.log(n_classes)
+ def _check_alpha(self):
Maybe you went through this before, but why isn't alpha just initially set
to an appropriate value and then used as before, instead of changing the
code a lot as below?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9131 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6yspSztoncgMuuTDYOBIqpTi5Oe-ks5sEc31gaJpZM4N7P0O>
.
|
Member
|
It wouldn't be modifying attributes, it would be checking alpha when passed into init and setting it appropriately initially. |
Contributor
Author
|
Not sure to understand your point but if we move the checking at clf = BernoulliNB(alpha=0.1)
clf.fit(X, y)
params={'alpha': 0.0}
clf.set_params(**params)
clf.fit(X, y) # Will crash |
Member
|
This looks good to me. Thanks for the contribution! |
jmschrei
approved these changes
Jun 19, 2017
This was referenced Jun 19, 2017
dmohns
pushed a commit
to dmohns/scikit-learn
that referenced
this pull request
Aug 7, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
dmohns
pushed a commit
to dmohns/scikit-learn
that referenced
this pull request
Aug 7, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
NelleV
pushed a commit
to NelleV/scikit-learn
that referenced
this pull request
Aug 11, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
paulha
pushed a commit
to paulha/scikit-learn
that referenced
this pull request
Aug 19, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
AishwaryaRK
pushed a commit
to AishwaryaRK/scikit-learn
that referenced
this pull request
Aug 29, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
maskani-moh
pushed a commit
to maskani-moh/scikit-learn
that referenced
this pull request
Nov 15, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
jwjohnson314
pushed a commit
to jwjohnson314/scikit-learn
that referenced
this pull request
Dec 18, 2017
…cikit-learn#9131) * Fix scikit-learn#5814 * Fix pep8 in naive_bayes.py:716 * Fix sparse matrix incompatibility * Fix python 2.7 problem in test_naive_bayes * Make sure the values are probabilities before log transform * Improve docstring of `_safe_logprob` * Clip alpha solution * Clip alpha solution * Clip alpha in fit and partial_fit * Add what's new entry * Add test * Remove .project * Replace assert method * Update what's new * Format float into %.1e * Update ValueError msg
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 Issue
Fixes #5814, continuation of #7477
What does this implement/fix? Explain your changes.
Move alpha setting into
fitandpartial_fit