You are welcome, thanks again for contributing =) - Henry
On Thu, Apr 10, 2014 at 3:17 PM, Ignacio Zendejas <ignacio.zendejas...@gmail.com> wrote: > I don't think there's a noticeable performance hit by the use of reverse in > those cases. It was a quick set of changes and it helped understand what > you look for. I didn't intend to nitpick, so I'll leave as is. I could have > used a scala.Ordering implicitly/explicitly also, but seems overkill and > don't want to necessarily start a discussion about what's best--unless one > of the admins deems this important. > > I'll only keep the use of take and tail over using slice and switch over to > math.min where indicated. > > This after I follow Henry's timely advice--thanks, Henry. > > cheers. > > > > > On Thu, Apr 10, 2014 at 2:10 PM, Reynold Xin <r...@databricks.com> wrote: > >> Thanks for contributing! >> >> I think often unless the feature is gigantic, you can send a pull request >> directly for discussion. One rule of thumb in the Spark code base is that >> we typically prefer readability over conciseness, and thus we tend to avoid >> using too much Scala magic or operator overloading. >> >> In this specific case, do you know if using - instead of reverse improve >> performance? I personally find it slightly awkward to use underscore right >> after negation ... >> >> >> The tail change looks good to me. >> >> For foldLeft, I agree with you that the old way is more readable (although >> less idiomatic scala). >> >> >> >> >> On Thu, Apr 10, 2014 at 1:48 PM, Ignacio Zendejas < >> ignacio.zendejas...@gmail.com> wrote: >> >> > Hi, all - >> > >> > First off, I want to say that I love spark and am very excited about >> > MLBase. I'd love to contribute now that I have some time, but before I do >> > that I'd like to familiarize myself with the process. >> > >> > In looking for a few projects and settling on one which I'll discuss in >> > another thread, I found some very minor optimizations I could contribute, >> > again, as part of this first step. >> > >> > Before I initiate a PR, I've gone ahead and tested style, ran tests, etc >> > per the instructions, but I'd still like to have someone quickly glance >> > over it and ensure that these are JIRA worthy. >> > >> > Commit: >> > >> > >> https://github.com/izendejas/spark/commit/81065aed9987c1b08cd5784b7a6153e26f3f7402 >> > >> > To summarize: >> > >> > * I got rid of some SeqLike.reverse calls when sorting by descending >> order >> > * replaced slice(1, length) calls with the much safer (avoids IOOBEs) and >> > more readable .tail calls >> > * used a foldleft to avoid using mutable variables in NaiveBayes code >> > >> > This last one is meant to understand what's valued more between idiomatic >> > Scala development or readability. I'm personally a fan of foldLefts where >> > applicable, but do think they're a bit less readable. >> > >> > Thanks, >> > Ignacio >> > >>
