naive_bayes/base.py (_many) methods to narwhals#1931
Conversation
|
if there are any changes i got it wrong do lemme know and for the testing i have test for the |
|
Ideally I'd like there to be tests to compare against sklearn. Also, you need to support Narwhals in the Honestly this is the kind of migration task that a coding agent excels at, I wouldn't shy away from using one! |
on it
oh ok got it that would speed things up thank you |
|
changes made |
Refactor return value to use original input X instead of x_nw.
Remove redundant return statement in Gaussian class.
Add missing newline at end of file.
Removed example usage and unnecessary comments from the GaussianNB class.
Removed print statements for column names in test.
Added examples for using the GaussianNB model with both single and mini-batch learning.
| @@ -0,0 +1,70 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
This file should be called test_gaussian.py, I would say
| assert np.allclose( | ||
| river_probs.to_numpy(), | ||
| skl_probs.to_numpy(), | ||
| atol=1e-2, |
There was a problem hiding this comment.
That's a high tolerance. Can you check why the tolerance can't be lower?
There was a problem hiding this comment.
np.max(np.abs(river_probs.to_numpy() - skl_probs.to_numpy())) using this i got 0.005864465204894698 , 1e-2 is the lowest
There was a problem hiding this comment.
Right, but can you check if this is an algorithmic issue? This can be indicative that the implementation is not exactly correct
There was a problem hiding this comment.
sklearn's GaussianNB uses population variance (ddof=0) because Gaussian Naive Bayes is based on maximum-likelihood estimation (MLE), where population variance is the correct estimator. Our implementation was using the library's default (ddof=1), which is intended for general statistical estimation. Instead of changing the library-wide default, I fixed it only in GaussianNB so it matches the algorithm while leaving the default behavior unchanged.
There was a problem hiding this comment.
Perfect, thank you, that's much better :)
2 participants
What does this PR do?
contributing to #1919
as the approach mentioned
steps 1,2,3,4 are done for the methods in the
BaseNB(joint_log_likelihood_many,predict_proba_many,pandas .subtract)AI writing disclosure
We welcome the use of AI tools to help with contributions. For transparency and to help us improve our review process, please indicate the level of AI involvement in this PR.