Skip to content

[MRG+1] correct comparison in GaussianNB for 'priors'#10005

Merged
TomDLT merged 2 commits intoscikit-learn:masterfrom
gxyd:check-priors
Oct 26, 2017
Merged

[MRG+1] correct comparison in GaussianNB for 'priors'#10005
TomDLT merged 2 commits intoscikit-learn:masterfrom
gxyd:check-priors

Conversation

@gxyd
Copy link
Copy Markdown
Contributor

@gxyd gxyd commented Oct 25, 2017 •
edited by TomDLT
Loading

Reference Issues/PRs

Fixes #9633

What does this implement/fix? Explain your changes.

correct comparison in GaussianNB for 'priors'

Any other comments?

I am not fully sure if I have written the test as correctly since, I have written it as clf.fit(X, Y) and I don't know if I should use assert_.... or not. That is why I have not put MRG on title of PR.

jnothman
jnothman approved these changes Oct 25, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I've not yet double-checked that this test fails on master (does it?), but this otherwise lgtm

sklearn/tests/test_naive_bayes.py
priors = np.array([0.08, 0.14, 0.03, 0.16, 0.11, 0.16, 0.07, 0.14, 0.11, 0.0])
Y = np.array([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])
clf = GaussianNB(priors)
clf.fit(X, Y)
Copy link
Copy Markdown
Member

@jnothman jnothman Oct 25, 2017

Choose a reason for hiding this comment

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

Comment "smoke test for issue #9633"

sklearn/tests/test_naive_bayes.py Outdated


def test_gnb_priors_sum_isclose():
"""Test whether the class prior sum is properly tested"""
Copy link
Copy Markdown
Member

@jnothman jnothman Oct 25, 2017

Choose a reason for hiding this comment

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

Use a comment rather than a docstring here. We do this because nose hides the function name when docstring is used.

@jnothman jnothman changed the title correct comparison in GaussianNB for 'priors' [MRG+1] correct comparison in GaussianNB for 'priors' Oct 25, 2017
@gxyd
Copy link
Copy Markdown
Contributor Author

gxyd commented Oct 26, 2017

I have addressed the comments and fixed the failing flake8 errors. Do I need to squash the commits? (I have 2 commits now) Was wondering if you follow some strict policy against 'squashing' public commits.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 26, 2017 via email

@gxyd
Copy link
Copy Markdown
Contributor Author

gxyd commented Oct 26, 2017 via email

@TomDLT
Copy link
Copy Markdown
Member

TomDLT commented Oct 26, 2017 •
edited
Loading

I've not yet double-checked that this test fails on master

Test does fail on master.

Squashed and merged, thanks @gxyd !

@TomDLT TomDLT merged commit e41c4d5 into scikit-learn:master Oct 26, 2017
@gxyd gxyd deleted the check-priors branch October 26, 2017 14:24
@gxyd
Copy link
Copy Markdown
Contributor Author

gxyd commented Oct 26, 2017

Thanks @TomDLT . This was my first contribution to scikit-learn.

@TomDLT
Copy link
Copy Markdown
Member

TomDLT commented Oct 26, 2017

Congrats on your first contribution, hoping to see you around again !

@gxyd
Copy link
Copy Markdown
Contributor Author

gxyd commented Oct 26, 2017

I'll be here.

srajanpaliwal pushed a commit to srajanpaliwal/scikit-learn that referenced this pull request Oct 26, 2017
@gxyd @srajanpaliwal
[MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#1…
abda14d 
…0005)
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 26, 2017

Forgot a changelog entry. @gxyd do you mind submitting a new PR with a change to doc/whats_new?

gxyd added a commit to gxyd/scikit-learn that referenced this pull request Oct 27, 2017
@gxyd
add changelog entry for fixed and merged PR scikit-learn#10005 issue s…
a62c0e6 
…cikit-learn#9633
@gxyd gxyd mentioned this pull request Oct 27, 2017
Merged
@gxyd
Copy link
Copy Markdown
Contributor Author

gxyd commented Oct 27, 2017

@jnothman see #10025 .

amueller pushed a commit that referenced this pull request Oct 27, 2017
@gxyd @amueller
[MRG+1] add changelog entry for fixed and merged PR #10005 issue #9633 (
aba87b7 
#10025)

* add changelog entry for fixed and merged PR #10005 issue #9633

* change name

* change PR number
donigian added a commit to donigian/scikit-learn that referenced this pull request Oct 28, 2017
@donigian
Merge branch 'master' of github.com:scikit-learn/scikit-learn into do…
3bdcb15 
…cs/donigian-update-contribution-guidelines

* 'master' of github.com:scikit-learn/scikit-learn: (23 commits)
  fixes scikit-learn#10031: fix attribute name and shape in documentation (scikit-learn#10033)
  [MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633 (scikit-learn#10025)
  [MRG] Fix LogisticRegression see also should include LogisticRegressionCV(scikit-learn#9995) (scikit-learn#10022)
  [MRG + 1] Labels of clustering should start at 0 or -1 if noise (scikit-learn#10015)
  MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573)
  [MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#10005)
  [MRG + 1] ENH add check_inverse in FunctionTransformer (scikit-learn#9399)
  [MRG] FIX bug in nested set_params usage (scikit-learn#9999)
  [MRG+1] Fix LOF and Isolation benchmarks (scikit-learn#9798)
  [MRG + 1] Fix negative inputs checking in mean_squared_log_error (scikit-learn#9968)
  DOC Fix typo (scikit-learn#9996)
  DOC Fix typo: x axis -> y axis (scikit-learn#9985)
  improve example plot_forest_iris.py (scikit-learn#9989)
  [MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClustering (scikit-learn#9875)
  DOC update news
  DOC Fix three typos in manifold documentation (scikit-learn#9990)
  DOC add missing dot in docstring
  DOC Add what's new for 0.19.1 (scikit-learn#9983)
  Improve readability of outlier detection example. (scikit-learn#9973)
  DOC: Fixed typo (scikit-learn#9977)
  ...
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@gxyd @maskani-moh
[MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#1…
6f6dd75 
…0005)
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@gxyd @maskani-moh
[MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 …
a8fe0d2 
…issue scikit-learn#9633 (scikit-learn#10025)

* add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633

* change name

* change PR number
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@gxyd
[MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#1…
126873c 
…0005)
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@gxyd
[MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 …
7518b2f 
…issue scikit-learn#9633 (scikit-learn#10025)

* add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633

* change name

* change PR number
@qinhanmin2014 qinhanmin2014 mentioned this pull request Jul 16, 2018
Closed
@qinhanmin2014 qinhanmin2014 mentioned this pull request Jun 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bad fp-comparison in check_priors (at least naive_bayes.py)

3 participants

@gxyd @jnothman @TomDLT