scholar icon indicating copy to clipboard operation
scholar copied to clipboard

Add CategoricalNB

Open ksew1 opened this issue 1 year ago • 5 comments

Added CategoricalNB :)

ksew1 avatar Dec 22 '24 20:12 ksew1

Hi @ksew1 and thanks for the pull request! Looks great on the first look and I will be having a detailed review soon.

krstopro avatar Dec 23 '24 09:12 krstopro

I've Dropped some minor comments, but LGMT. There are some part that are common for all of NB algorithms. I think that it will be cool to separate them into utils.ex inside naive_bayes dir. But it's not necessary as a scope of this PR.

I’ve addressed all the comments 😄. I agree that it would be a good idea to extract the common parts into a separate file. I’ll do this in a separate PR.

ksew1 avatar Dec 31 '24 13:12 ksew1

Any reason why num_categories isn't a required option?

krstopro avatar Jan 02 '25 12:01 krstopro

Any reason why num_categories isn't a required option?

num_categories isn’t needed as an option since it’s just an implementation detail. We can easily calculate it from min_categories, which the user can already specify, so i think adding it would just be redundant

ksew1 avatar Jan 02 '25 22:01 ksew1

@josevalim @msluszniak I am still having a look. Seems very good, but please give me some time before merging. I might have some improvements to suggest (e.g. maybe the code can be vectorised; this could be valuable for @ksew1 to learn in case they don't know it already).

Happy New Year everyone!

krstopro avatar Jan 04 '25 11:01 krstopro