igrigorik / decisiontree

ID3-based implementation of the ML Decision Tree algorithm
1.44k stars 130 forks source link

Making code easier to read #26

Closed dvisockas closed 8 years ago

dvisockas commented 9 years ago

Been reading source for the last few hours so thought I'd make it easier to read. :heart:

ghost commented 9 years ago

This should be an instamerge!

igrigorik commented 9 years ago

I'm ok to merge this, but not sure that spaces are making it that much easier... I'll be the first to admit that the code is a bit of a mess and could really use some love! Ideally, a rubocop-passing reformat -- that would be awesome. Any chance you'd be up for that? :-)

ghost commented 9 years ago

+1 for rubocop, thats an awesome idea.

2015-06-26 16:19 GMT+02:00 Ilya Grigorik notifications@github.com:

I'm ok to merge this, but not sure that spaces are making it that much easier... I'll be the first to admit that the code is a bit of a mess and could really use some love! Ideally, a rubocop-passing reformat -- that would be awesome. Any chance you'd be up for that? :-)

— Reply to this email directly or view it on GitHub https://github.com/igrigorik/decisiontree/pull/26#issuecomment-115704365 .

dvisockas commented 9 years ago

I will be forking it anyway, so the answer is yes :)

dvisockas commented 8 years ago

So I've ran through id3_tree.rb file and tried to fix as much as possible. Though I didn't fix "Cyclomatic complexity for predict is too high", "Method too long", "Line too long" and few other offences, since they require more time.

Should I just keep commiting on this PR and when I'm done you'll just merge it? :)

igrigorik commented 8 years ago

Should I just keep commiting on this PR and when I'm done you'll just merge it? :)

Yup! And thanks for doing this :)

dvisockas commented 8 years ago

I guess I'm done :)

igrigorik commented 8 years ago

The examples appear to be broken :(

dvisockas commented 8 years ago

They're working now :fireworks:

igrigorik commented 8 years ago

There's still the type conversion bug:

igrigorik { /git/decisiontree/examples  git:(upstream ⚡ dvisockas-master) 2A} > ruby -I../lib simple.rb
Predicted: sick ... True decision: sick
/Users/igrigorik/git/decisiontree/lib/decisiontree/id3_tree.rb:193:in `gsub!': no implicit conversion of Float into String (TypeError)
dvisockas commented 8 years ago

Fixed :hammer:

igrigorik commented 8 years ago

Great stuff, thanks!