Hi Matt, Thank you for the code review!
> gradboostreg/tree, stat, sample have no tests. Yes, thanks for reminder, I should write tests for them! > These sub-packages may be better in package gradboostreg since they seem straightforward and contained to this library. Keeping those things in sub-packages doesn’t seem to add much value over just having separate files in my opinion. I put them in separate packages in an attempt to structure the code to be readable and isolate the intermediary functions and show that they are only helper funcs by unexporting them. I know it's not interesting to have a package with one file and that only one other package using it, but the benefits outweigh the uninterestingness for me. Maybe because I have no problem jumping around the files. > If you do keep the sub-packages I suggest changing sample.DefaultSample to just sample.Default. Great idea! I ran out of idea what to call it. > I’m not sure I understand why tree.Decider as an interface is necessary. The only use in the package is to return a Decider from NewDecisionTree. I would define such an interface where it’s used. I was thinking that you can implement deciders other than decision trees, e.g. a Gaussian decider! > Do you intend to add an open source license? If not, be sure to read the GitHub default license. I have no idea or preferences about licenses, except that I felt that MIT looked like the most permissive one. I will add a license later today. Thanks again for your time reviewing the project :) Sina On Tue, Jan 30, 2018 at 3:28 AM, <matthewju...@gmail.com> wrote: > > Hi Sina, here’s a general code review. > > gradboostreg/tree, stat, sample have no tests. > > These sub-packages may be better in package gradboostreg since they seem straightforward and contained to this library. Keeping those things in sub-packages doesn’t seem to add much value over just having separate files in my opinion. > > If you do keep the sub-packages I suggest changing sample.DefaultSample to just sample.Default. > > I’m not sure I understand why tree.Decider as an interface is necessary. The only use in the package is to return a Decider from NewDecisionTree. I would define such an interface where it’s used. > > The godoc has no documentation. > > Do you intend to add an open source license? If not, be sure to read the GitHub default license. > > Matt > > On Monday, January 29, 2018 at 4:43:38 PM UTC-6, Sina Siadat wrote: >> >> Hi Sebastien, >> >> Thanks for your comment and question :) >> >> > I have one "drive-by-comment" and a question: >> > you could have perhaps used gonum for the stats stuff :) >> >> Actually, I did start with gonum :)) but I thought it was a large dependency and I only needed a few funcs from it, so I decided to copy/write them! >> >> > and the question: did you compare your package with XGBoost (which is now kind of a standard candle nowadays) in terms of accuracy, speed and memory usage? >> >> I haven't used XGBoost yet, it looks pretty cool, thanks for mentioning it. I will install it and see if I can compare the two. >> >> Cheers! >> Sina >> > -- > You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group. > To unsubscribe from this topic, visit https://groups.google.com/d/ topic/golang-nuts/bi7WEHeeNWs/unsubscribe. > To unsubscribe from this group and all its topics, send an email to golang-nuts+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
