8000 Remove duplicate GaussianNB.fit() code by ihaque · Pull Request #3344 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

Remove duplicate GaussianNB.fit() code #3344

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

Merged
merged 3 commits into from
Jul 5, 2014

Conversation

ihaque
Copy link
Contributor
@ihaque ihaque commented Jul 4, 2014

Follow-on to #3324.

GaussianNB.fit is substantially duplicated by partial_fit, and the latter is more flexible, so just have the former call the latter. There should be no significant degradation in performance or numerical stability, since the mean/variance update will just call numpy.mean and numpy.var when n_past == 0 rather than trying anything fancy.

This code is substantially duplicated by partial_fit(), so just call
partial_fit().
@@ -235,7 +215,7 @@ def _update_mean_variance(n_past, mu, var, X):

return total_sum / n_total, total_ssd / n_total

def partial_fit(self, X, y, classes=None):
def partial_fit(self, X, y, classes=None, _refit=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding such a param is not ok. The API shall stay clean. You might rely a private _partial_fit method.

@ihaque
Copy link
Contributor Author
ihaque commented Jul 4, 2014

OK, preserved API using your suggestion in d709833.

Must be provided at the first call to partial_fit, can be omitted
in subsequent calls.

_refit: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean -> bool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 2d12952

@agramfort
Copy link
Member
8000

LGTM if travis is happy

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling 2d12952 on ihaque:remove_dup_fit into 08b2902 on scikit-learn:master.

agramfort added a commit that referenced this pull request Jul 5, 2014
Remove duplicate GaussianNB.fit() code
@agramfort agramfort merged commit 1775095 into scikit-learn:master Jul 5, 2014
@agramfort
Copy link
Member

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0