-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adaboost Classifier :- Stopped iterations when sample weights overflow and print a warning message #10096
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take care of pep8
sklearn/ensemble/weight_boosting.py
Outdated
@@ -145,7 +145,11 @@ def fit(self, X, y, sample_weight=None): | |||
random_state) | |||
|
|||
# Early termination | |||
if sample_weight is None: | |||
|
|||
if sample_weight is None and math.isnan(estimator_error): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to test sample_weight twice.
if sample_weight is None:
if math.isnan(estimator_error):
do_something_specific
break
sklearn/ensemble/weight_boosting.py
Outdated
if sample_weight is None: | ||
|
||
if sample_weight is None and math.isnan(estimator_error): | ||
print("Underflow of weighted error occured during iterations! Iterations stopped ! High chances of Overfitting!, Try decreasing the learning rate or n_estimators to avoid this! ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use proper warning messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and lines should be <80 characters
sklearn/ensemble/weight_boosting.py
Outdated
@@ -497,6 +501,9 @@ def _boost_real(self, iboost, X, y, sample_weight, random_state): | |||
# Error fraction | |||
estimator_error = np.mean( | |||
np.average(incorrect, weights=sample_weight, axis=0)) | |||
if math.isnan(estimator_error): | |||
return None,None,estimator_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after commas (otherwise is not pep8)
sklearn/ensemble/weight_boosting.py
Outdated
@@ -552,6 +559,8 @@ def _boost_discrete(self, iboost, X, y, sample_weight, random_state): | |||
# Error fraction | |||
estimator_error = np.mean( | |||
np.average(incorrect, weights=sample_weight, axis=0)) | |||
if math.isnan(estimator_error): | |||
return None,None,estimator_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "idem" mean? Sorry if the question is lame.
wellcome. Tag your title [WIP] and [MRG] when you are ready for revision. Thx |
sklearn/ensemble/weight_boosting.py
Outdated
if sample_weight is None: | ||
if estimator_error is not None and math.isnan(estimator_error): | ||
print("Early termination due to underflow of estimated_error.Iterations stopped") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use print
statements for warning. (see contributing guide for a warning example)
Or see this example in the code:
scikit-learn/sklearn/preprocessing/data.py
Lines 180 to 185 in 3e85359
if not np.allclose(mean_2, 0): | |
warnings.warn("Numerical issues were encountered " | |
"when scaling the data " | |
"and might not be solved. The standard " | |
"deviation of the data is probably " | |
"very close to 0. ") |
More over if you introduce a if statement, the code is not tested therefore a test needs to be written. See the test of the previous example here:
scikit-learn/sklearn/preprocessing/tests/test_data.py
Lines 219 to 220 in 3e85359
w = "standard deviation of the data is probably very close to 0" | |
x_scaled = assert_warns_message(UserWarning, w, scale, x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll work on the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@massich I am not so sure how a good test for this would be like
A separate function for testing?
Heres a snapshot. Any suggestions would be great,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no place where something similar is tested, create a new function. But I would use a more descriptive name like test_early_stop_when_estimator_error_bcomes_nan
. I would also add a reference to the original issue.
To avoid PEP8 errors, I would also set the warning message into a variable to further compare to. (And make sure that the warning raised and catch are the same (try to find a name bettern than w
or early_stop_warnmsg
)
early_stop_warnmsg = "Early termination due to underflow of estimated_error. Iterations stopped"
assert_warns_message(UserWarning, early_stop_warnsg, clf.fit, iris.data, iris.target)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks, I will make these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@massich I don't think the estimator error is underflowing. if it is underflowing then won't estimated error be approximated to zero? What I suppose is that sample_weight
are shooting up to infinite when summed(probably due to high learning rate). Causing inf/inf division for estimated error. Giving a nan. Any suggestions?
@massich Any changes needed? Actually there is no underflow of estimator but sample weights reaching infinite values are causing the Nan. The original issue title is not what the problem is. So should I change my PR title? |
Yes, please update the PR title. Does normalizing the If this is about numerical precision, it might also help to perform the boosting in log space, so using |
We do normalise the sample weight after each boosting step , which requires summing up all the sample weights, and thats where we get 'inf'. |
You're right, we do. Yes, please do try log-sum-exp. |
@jnothman Converting to log-space isn't working well ,it is affecting accuracy and causing test_iris() to fail due to low accuracy. So I think its better to generate a warning and stop iterations. |
@Fenil3510 that sounds odd. Can you please commit the change so we can review? |
@amueller please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted to log-space I hope the conversion is right.
sklearn/ensemble/weight_boosting.py
Outdated
@@ -581,7 +581,8 @@ def _boost_discrete(self, iboost, X, y, sample_weight, random_state): | |||
# Only boost the weights if I will fit again | |||
if not iboost == self.n_estimators - 1: | |||
# Only boost positive weights | |||
sample_weight *= np.exp(estimator_weight * incorrect * | |||
sample_weight = np.log(np.exp(sample_weight) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the exp and the log the wrong way around. You want to turn a product into a sum of logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok,
sample_weight = np.log(sample_weight * np.exp(estimator_weight*incorrect....))
This is fine I guess.
I'll commit the changes if the above is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you need to exp(log(sample_weight) + est_weight * incorrect * ...)
sklearn/ensemble/weight_boosting.py
Outdated
@@ -581,7 +581,7 @@ def _boost_discrete(self, iboost, X, y, sample_weight, random_state): | |||
# Only boost the weights if I will fit again | |||
if not iboost == self.n_estimators - 1: | |||
# Only boost positive weights | |||
sample_weight = np.log(np.exp(sample_weight) + | |||
sample_weight = np.exp(np.log(sample_weight) * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, you need a sum of logs. That * should be a +.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! I am really sorry. will fix
Well Travis gave you green for everything but flake8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that still means we're getting overflow...
@jnothman Yes there is still an overflow. Anything else we could try? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @fenilsuchak !
Please add an entry to the change log at doc/whats_new/v0.24.rst
with tag |Fix|. Like the other entries there, please reference this pull request with :pr:
and credit yourself (and other contributors if applicable) with :user:
.
sklearn/ensemble/weight_boosting.py
Outdated
sample_weight *= np.exp(estimator_weight * incorrect * | ||
((sample_weight > 0) | | ||
(estimator_weight < 0))) | ||
sample_weight = np.exp(np.log(sample_weight) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's include a comment here explaining why we are doing this in log space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we had an advantage doing this we still had an overflow.
w = "Sample weights have reached infinite values" | ||
clf = AdaBoostClassifier(n_estimators=30, learning_rate=5., | ||
algorithm="SAMME") | ||
assert_warns_message(UserWarning, w, clf.fit, iris.data, iris.target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been moving toward using pytest.warns
:
msg = "Sample weights have reached infinite values"
with pytest.warns(UserWarning, match=msg):
clf.fit(iris.data, iris.target)
@cmarmo not sure why the tests failing. |
Not sure either, but it's failing on upstream/master too with the same error... |
It's not the same issue but it's a compilation on macOS issue as usual :) |
Ok, will wait for the merge of #17913 then to rerun tests again? |
Hi @thomasjpfan this is a three year old PR ... do you mind checking if your comments have been addressed? The check failure is unrelated with the PR itself. Thanks! |
sklearn/ensemble/_weight_boosting.py
Outdated
sample_weight = np.exp(np.log(sample_weight) + | ||
estimator_weight * incorrect * | ||
((sample_weight > 0) | | ||
(estimator_weight < 0))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason behind estimator_weight < 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks like a backward incompatible change to me. I reverted this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fenilsuchak ! I fixed conflicts and added a changelog entry.
Co-authored-by: Fenil Suchak <fenilsuchak@fenil.local> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Fixes #10077
Error fixed in adaboost classifier.At some iterations weighted error was underflowing, due to high learning rate.,Making error as NaN and hence making subsequent iterations useless.
This update halts the iterations when such warning is encountered and prints a warning message to inform about the underflow.
This is my first PR. I am open to all sorts of criticism.