Skip to content
This repository was archived by the owner on Jul 16, 2021. It is now read-only.

example: classifying dogs with a naïve Bayes model#131

Closed
zackmdavis wants to merge 1 commit intoAtheMathmo:masterfrom
zackmdavis:naïve_dogs

Hidden character warning

The head ref may contain hidden characters: "na\u00efve_dogs"
Closed

example: classifying dogs with a naïve Bayes model#131
zackmdavis wants to merge 1 commit intoAtheMathmo:masterfrom
zackmdavis:naïve_dogs

Conversation

@zackmdavis
Copy link
Copy Markdown
Collaborator

This in the matter of #128.

@zackmdavis
Copy link
Copy Markdown
Collaborator Author

... except that CI doesn't know that the example needs the stats feature (see discussion on #130).

@AtheMathmo
Copy link
Copy Markdown
Owner

I love the context for the example! I have a few comments for the example code. I think it needs some more inline comments and hand holding. Maybe we can try to separate out the data-creation and the machine learning parts too. But really it only needs some mild changes.

I guess the bigger issue is the feature flag... I'll see if I can find a way to fix that using the stats feature (maybe we can hide the example behind the feature flag?). Otherwise it might be best to just use the rand crate here for the Gaussians?

@zackmdavis
Copy link
Copy Markdown
Collaborator Author

OK; sometime in the next few days I'll add comments and change it to use rand::distributions::normal::Normal

@AtheMathmo
Copy link
Copy Markdown
Owner

AtheMathmo commented Sep 21, 2016

OK; sometime in the next few days I'll add comments and change it to use rand::distributions::normal::Normal

Great, thank you!

I've merged in #133 which has some breaking changes (going into the 0.5.0 release). Let me know if you need any help resolving. It's mostly modifications to trait imports and some behind-the-scenes naive bayes changes (shouldn't affect you).

@zackmdavis
Copy link
Copy Markdown
Collaborator Author

(updated and force-pushed)

Copy link
Copy Markdown
Owner

@AtheMathmo AtheMathmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a lot better (perhaps good enough) but I still feel like we need a little more explanation. Do you think I'm being a bit too cautious?

if accurate {
hits += 1;
}
println!("Predicted: {:?}; Actual: {:?}; Accurate? {:?}",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to print so many lines to our users terminal - maybe we just store the last 10 or so and print these?

}

// Train!
let mut model = NaiveBayes::<naive_bayes::Gaussian>::new();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give some more information to the reader here (briefly of course). Why do we choose Gaussian?

Comment thread examples/README.md

#### Dog Classification

Suppose we have a population composed of red dogs and white dogs, whose friendliness, furriness, and speed can be measured. The group of white dogs is friendlier, furrier, and slower than red dogs by one standard deviation (respectively), but given the color of a dog, friendliness, furriness, and speed are independent of each other. We can use a naïve Bayes model to try to predict dog color given friendliness, furriness, and speed as observations.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think another paragraph with a little more technical detail would be good. In particular I think it's worthwhile to address:

  • What kind of Naive Bayes' do we use? Why?
  • As the example is quite large - maybe walk through roughly what we do? Randomly generate some dogs, convert them into Matrix form for rusty-machine. Create model, train model, predict from model on unknown dogs. Assess accuracy.

What do you think? Perhaps there is already enough information there for new users - it certainly makes sense to me but I'm not a good reference point!

@zackmdavis
Copy link
Copy Markdown
Collaborator Author

OK; more commentary later

@AtheMathmo
Copy link
Copy Markdown
Owner

@zackmdavis did you need anything from me for this?

Not rushing you at all - take as long as you like!

@zackmdavis
Copy link
Copy Markdown
Collaborator Author

Alternatively, merge now and accept improvements later if I (or—we can unrealistically hope—someone else) happen to get around to it??

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants