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
FIX do not report wrong iteration number when initialization of EM does not converge #26837
Conversation
Hi @krstopro, thanks for opening the Pull Request. Could you please add a short section to your description that explains "What does this implement/fix?" as well as explaining your changes. The title also needs improving to be more descriptive. All this helps people know what the PR is about, what needs doing, etc which will reduce the time it takes for things to get reviewed and merged. |
sklearn/mixture/_base.py
Outdated
"""Print verbose message on the end of iteration.""" | ||
if self.verbose == 1: | ||
print("Initialization converged: %s" % self.converged_) | ||
print("Initialization converged: %s" % converged) |
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.
The message will look weird because we will get: "Initialization converged: False"
It would be better to report that the convergence did not happen using a proper English sentence based on converged
.
Also I would prefer init_has_converged
instead of converged
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.
However, we can mention that the "Initialization ended".
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 should not have strong test regarding the verbosing. It would make sense to strengthen test_gaussian_mixture_verbose
.
We also need a what's new entry in 1.4 to acknowledge that the error message is fixed. |
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.
I pushed the requested changes and fixed the doc of the converged_
attribute.
LGTM on my side.
@glemaitre wanna have another look ?
@jeremiedbb I'll have a new look. Thanks |
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.
LGTM. Thanks @jeremiedbb and @krstopro
Fixes #26621
When the EM best iteration does not converge, a warning message is raised, indicating a wrong iteration number. As discussed in #26621, we don't need to indicate the number but rather mention that the convergence did not happen.