Thanks for sharing gradboostreg here. Matt
On Tuesday, January 30, 2018 at 12:47:33 AM UTC-6, Sina Siadat wrote: > > 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, <matthe...@gmail.com <javascript:>> > 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...@googlegroups.com <javascript:>. > > 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.
