Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

In response to Issue #67: Naive Bayes Classifier added along with a unit testing file; also, included a test_use_cases.py file to compare the accuracy of naive bayes models using both numpy-ml and scikit-learn using dataset in the file wine.data #75

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

Open
rishabh-jain14 wants to merge 1 commit into ddbourgin:master
base: master
Choose a base branch
Loading
from rishabh-jain14:master

Conversation

@rishabh-jain14
Copy link

@rishabh-jain14 rishabh-jain14 commented Aug 31, 2021

All Submissions

  • [Yes] Is the code you are submitting your own work?
  • [Yes] Have you followed the contributing guidelines?
  • [Yes] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Model Submissions

  • [Yes] Is the code you are submitting your own work?
  • [Yes] Did you properly attribute the authors of any code you referenced?
  • [Yes] Did you write unit tests for your new model?
  • [Yes] Does your submission pass the unit tests?
  • [No] Did you write documentation for your new model?
  • [No] Have you formatted your code using the black deaults?

Changes to Existing Models

  • [Yes] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [Yes] Have you written new tests for your changes, as applicable?
  • [Yes] Have you successfully ran tests with your changes locally?

@rishabh-jain14 rishabh-jain14 changed the title (削除) Naive Bayes Classifier added along with a unit testing file; also, included a test_use_cases.py file to compare the accuracy of naive bayes models using both numpy-ml and scikit-learn using dataset in the file wine.data (削除ここまで) (追記) In response to Issue #67: Naive Bayes Classifier added along with a unit testing file; also, included a test_use_cases.py file to compare the accuracy of naive bayes models using both numpy-ml and scikit-learn using dataset in the file wine.data (追記ここまで) Aug 31, 2021
Copy link
Owner

Hi Rishabh, thanks for the PR, and sorry for my delayed response. Also apologies for not closing issue #67 after @sfsf9797 's PR. I suspect this may have created some confusion :-/

Can you explain what your code adds over the existing implementation? I haven't gone through it in detail, but given that there already exists a validated Gaussian naive Bayes model in the repo, I'm inclined to leave things as they are unless there is significant added functionality.

Copy link
Author

rishabh-jain14 commented Sep 23, 2021 via email

Hey David, thanks for the response. The PR focuses on the implementation and comparison of the Gaussian Naive Bayes Classifier using the NumPy-ml repo as well as the scikit-learn library. I had realized that the repo already contains a code for the Gaussian naive Bayes model but as per the issue mentioned #67, it required a brief coded file that focused on the comparison of the performance of the model in comparison to the already specified scikit-learn model. So, we can say that the comparison part is the significant added functionality here. Feel free to include the PR into the repo if you'd like. I enjoyed assessing the entire repository and look forward to contributing as much as I can. :-) *Rishabh Jain*
...
On Thu, Sep 23, 2021 at 9:04 PM David Bourgin ***@***.***> wrote: Hi Rishabh, thanks for the PR, and sorry for my delayed response. Also apologies for not closing issue #67 <#67> after @sfsf9797 <https://github.com/sfsf9797> 's PR. I suspect this may have created some confusion :-/ Can you explain what your code adds over the existing implementation? I haven't gone through it in detail, but given that there already exists a validated Gaussian naive Bayes model in the repo, I'm inclined to leave things as they are unless there is significant added functionality. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANHP7HB3YYKK6WHSMC76T6TUDNCJ3ANCNFSM5DD2B7IQ> .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /