Reorg/naive bayes #145
Reorg/naive bayes #145
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request involves significant modifications to the Naive Bayes classification framework. It includes the deletion of several test files related to categorical and multinomial Naive Bayes classifiers, while enhancing the testing capabilities for Naive Bayes classifiers in a consolidated test file. A new implementation file introduces an abstract base class for various Naive Bayes classifiers, including Gaussian, Multinomial, and Categorical variants. The changes also remove previous implementations of these classifiers, streamlining the codebase. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
tests/classification/test_naive_bayes.py (3)
109-109: Misleading comment about variance calculationThe comment
# use the same variance calculation config with sklearnis not applicable here becauseMultinomialNBdoes not involve variance calculations. This comment might cause confusion.Consider removing or updating the comment to accurately reflect the configuration being matched with scikit-learn.
- # use the same variance calculation config with sklearn
139-139: Misleading comment about variance calculationThe comment
# use the same variance calculation config with sklearnis not relevant forCategoricalNB, as it does not involve variance calculations. This could lead to misunderstandings.Consider removing or correcting the comment to accurately represent the configuration alignment with scikit-learn.
- # use the same variance calculation config with sklearn
51-51: Align class labels with zero-based indexingThe class labels are currently set as
[1, 2, 3, 4, 5, 6]. It's conventional in Python and scikit-learn to use zero-based indexing for class labels.Consider updating the labels to start from
0to adhere to common practices and prevent potential indexing issues.- label = np.array([1, 2, 3, 4, 5, 6]).tolist() + label = np.array([0, 1, 2, 3, 4, 5]).tolist()After making this change, ensure all related code, especially indexing in probability arrays, is adjusted accordingly.
toyml/classification/naive_bayes.py (1)
321-325: Specify precise return type annotations for_get_classes_feature_count_probmethodThe return type of the
_get_classes_feature_count_probmethod is currently annotated astuplewith a type ignore comment. To enhance type checking and code clarity, specify the exact return types.Apply this diff to update the return type annotations:
def _get_classes_feature_count_prob( self, dataset: list[list[FeatureValue]], labels: list[Class], - ) -> tuple: # type: ignore[type-arg] + ) -> tuple[ + dict[Class, dict[Dimension, dict[FeatureValue, float]]], + dict[Class, dict[Dimension, dict[FeatureValue, float]]] + ]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
tests/classification/naive_bayes/test_categorical_naive_bayes.py(0 hunks)tests/classification/naive_bayes/test_multinomial_naive_bayes.py(0 hunks)tests/classification/test_naive_bayes.py(3 hunks)toyml/classification/naive_bayes.py(1 hunks)toyml/classification/naive_bayes/__init__.py(0 hunks)toyml/classification/naive_bayes/categorical_naive_bayes.py(0 hunks)toyml/classification/naive_bayes/gaussian_naive_bayes.py(0 hunks)toyml/classification/naive_bayes/multinomial_naive_bayes.py(0 hunks)
💤 Files with no reviewable changes (6)
- tests/classification/naive_bayes/test_categorical_naive_bayes.py
- tests/classification/naive_bayes/test_multinomial_naive_bayes.py
- toyml/classification/naive_bayes/init.py
- toyml/classification/naive_bayes/categorical_naive_bayes.py
- toyml/classification/naive_bayes/gaussian_naive_bayes.py
- toyml/classification/naive_bayes/multinomial_naive_bayes.py
🔇 Additional comments (1)
toyml/classification/naive_bayes.py (1)
164-164: Verify Python version compatibility with zip(..., strict=True) usage
The strict=True argument in the zip function (lines 164, 170, 260, 355) requires Python 3.10 or later. If the project is intended to support earlier Python versions, this will lead to compatibility issues. Please confirm the minimum required Python version for the project.
To check the current Python version specified in the project, run the following script:
Also applies to: 170-170, 260-260, 355-355
✅ Verification successful
Based on the output, I can generate the final response as we have clear evidence about the Python version compatibility.
zip(..., strict=True) usage is compatible with project's Python version requirements
The project explicitly declares support for Python 3.10 and above in pyproject.toml, which aligns perfectly with the usage of zip(..., strict=True) parameter introduced in Python 3.10. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Python version specified in the project configuration
# Check for a pyproject.toml or setup.cfg file specifying the Python version
fd -e toml -e cfg --type f "pyproject.toml" "setup.cfg" --exec cat {}
# Alternatively, search for classifiers in setup.py
rg "Programming Language :: Python ::"
# Expected: Python version 3.10 or higher specified
Length of output: 533
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #145 +/- ##
==========================================
- Coverage 98.31% 98.15% -0.16%
==========================================
Files 15 12 -3
Lines 1008 975 -33
==========================================
- Hits 991 957 -34
- Misses 17 18 +1 ☔ View full report in Codecov by Sentry. |
Related to #121
Summary by CodeRabbit
Bug Fixes
New Features
Documentation