Skip to content
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

[ENH] Naive Bayes: Implement predict, fix predict_storage #3540

Merged
merged 4 commits into from
Jan 28, 2019

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Jan 17, 2019

Implement predict to:

  • avoid numpy warning about passing generator to sum
  • loop over columns instead of over rows (usually better + more friendly to pandas sometime in the future)
  • natively support sparse matrices

predict_storage now calls predict when possible.

predict_storage also didn't work for Instance. Fixed.

Includes
  • Code changes
  • Tests

@janezd janezd mentioned this pull request Jan 17, 2019
1 task
@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #3540 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3540      +/-   ##
==========================================
+ Coverage   83.95%   83.97%   +0.01%     
==========================================
  Files         370      370              
  Lines       66876    66920      +44     
==========================================
+ Hits        56146    56193      +47     
+ Misses      10730    10727       -3

@janezd janezd force-pushed the reimplement-nb-predict-storage branch from 6116c1b to 49a5b4a Compare January 18, 2019 14:39
@janezd janezd changed the title [RFC] Naive Bayes: Reimplement predict_storage [ENH] Naive Bayes: Implement predict, fix predict_storage Jan 18, 2019
@janezd janezd force-pushed the reimplement-nb-predict-storage branch from 49a5b4a to 0dddfc8 Compare January 18, 2019 15:06
@markotoplak
Copy link
Member

Ok, I like the change.

But why did you decide not route Instance (transformed to Table now) through predict also? If I do it, I see that code to compute probabilities in predict_storage is not run anymore. :)

We should hardcode some actual probabilities into tests. We do not have any test that checks actual results (comparing CA does not count; these tests should be removed, I think).

@janezd janezd force-pushed the reimplement-nb-predict-storage branch 4 times, most recently from 357861e to 33b6e55 Compare January 25, 2019 21:44
Copy link
Contributor Author

@janezd janezd left a comment

Choose a reason for hiding this comment

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

@markotoplak, I routed Instance through predict and wrote tests that actually check probabilities against those I computed manually.

Orange/classification/naive_bayes.py Show resolved Hide resolved
Orange/classification/naive_bayes.py Show resolved Hide resolved
Orange/tests/test_naive_bayes.py Show resolved Hide resolved
Orange/tests/test_naive_bayes.py Show resolved Hide resolved
@janezd
Copy link
Contributor Author

janezd commented Jan 25, 2019

I removed almost all existing tests but I kept the testing of CA as an (admittedly insensitive) sanity check.

@markotoplak
Copy link
Member

Good to have good tests (I did not redo your calculations though), thanks.

It must have been a little late while you were coding this. :) I added two commits. I added the code from the last commit just for my testing, but then I added it, because it makes tests more explicit.

@@ -48,10 +48,10 @@ def __init__(self, log_cont_prob, class_prob, domain):
self.class_prob = class_prob

def predict_storage(self, data):
if isinstance(data, Instance):
data = Table(np.atleast_2d(data.x))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I somehow thought I've done this correctly. (But I have no good explanation for assert_not_called in tests. :))

@markotoplak
Copy link
Member

Please check if I did something stupid and then merge at will (you can also rebase if you want).

@janezd janezd merged commit c9e60c9 into biolab:master Jan 28, 2019
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.

2 participants