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

Bug in MultinomialNB? Incorrect calculation of P(x|c). #42

Closed
ablaom opened this issue Mar 4, 2019 · 2 comments
Closed

Bug in MultinomialNB? Incorrect calculation of P(x|c). #42

ablaom opened this issue Mar 4, 2019 · 2 comments

Comments

@ablaom
Copy link
Collaborator

ablaom commented Mar 4, 2019

I am struggling to understand the code below from NaiveBayes.jl/src/multinomial.jl. It seems to me that the denominator in x_priors_for_c = m.x_counts[c] ./ m.x_totals is incorrect. Rather, the denominator should be sum(m.x_counts[c]), no?

Since the test code for MultinomialNB is not very thorough, this could easily have been missed.

@dfdx Assuming you agree this is a bug, if I create a PR with a correction, plus rigorous test, would you consider reviewing in near future?

"""Calculate log P(x|C)"""
function logprob_x_given_c(m::MultinomialNB, x::Vector{Int64}, c::C) where C
    x_priors_for_c = m.x_counts[c] ./ m.x_totals
    x_probs_given_c = x_priors_for_c .^ x
    logprob = sum(log(x_probs_given_c))
    return logprob
end
@dfdx
Copy link
Owner

dfdx commented Mar 4, 2019

Yes, this package was created in prehistoric times and has never been seriously reviewed since then, so it's pretty possible it contains even trivial bugs.

I might have had something different in my mind at the time of writing, but looking at the code now I believe you are right. This may also be the reason for #40.

If you post a PR fixing this I'll try to review it within 12 hours.

@ablaom
Copy link
Collaborator Author

ablaom commented Sep 28, 2020

Okay, I'm going to try and get a PR for this during the next week.

ablaom added a commit to ablaom/NaiveBayes.jl that referenced this issue Sep 29, 2020
dfdx added a commit that referenced this issue Sep 29, 2020
@ablaom ablaom closed this as completed Mar 25, 2021
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

No branches or pull requests

2 participants