You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mahout.apache.org by Jake Mannix <ja...@gmail.com> on 2013/05/24 04:06:29 UTC
Comments on MAHOUT-1227 ?
It's done, patch passes tests, but it involves adding 308 lines
(removing 556 others!) across 84 classes in the codebase, so it probably
touches something you wrote.
So I'm going to let this diff marinade for a day or so, but if you feel
like you've got some thoughts (+1's? -1's? I'll even take some +epsilons
if you're feeling cowardly) on it, feel free to comment on the
ticket<https://issues.apache.org/jira/browse/MAHOUT-1227> or
on the review <https://reviews.apache.org/r/11359> itself.
I won't let the patch stick around forever though, as most every commit
that hits trunk will force me to regenerate this patch again, given its
breadth.
--
-jake
Re: Comments on MAHOUT-1227 ?
Posted by Jake Mannix <ja...@gmail.com>.
This has been submitted.
I suggest everyone who's got changes checked out update sometime soon, to
minimize merge conflicts.
On Fri, May 24, 2013 at 2:17 AM, Shannon Quinn <sq...@gatech.edu> wrote:
> LGTM!
>
>
> On 5/23/13 10:06 PM, Jake Mannix wrote:
>
>> It's done, patch passes tests, but it involves adding 308 lines
>> (removing 556 others!) across 84 classes in the codebase, so it probably
>> touches something you wrote.
>>
>> So I'm going to let this diff marinade for a day or so, but if you feel
>> like you've got some thoughts (+1's? -1's? I'll even take some +epsilons
>> if you're feeling cowardly) on it, feel free to comment on the
>> ticket<https://issues.apache.**org/jira/browse/MAHOUT-1227<https://issues.apache.org/jira/browse/MAHOUT-1227>>
>> or
>> on the review <https://reviews.apache.org/r/**11359<https://reviews.apache.org/r/11359>>
>> itself.
>>
>>
>> I won't let the patch stick around forever though, as most every commit
>> that hits trunk will force me to regenerate this patch again, given its
>> breadth.
>>
>>
>
--
-jake
Re: Comments on MAHOUT-1227 ?
Posted by Shannon Quinn <sq...@gatech.edu>.
LGTM!
On 5/23/13 10:06 PM, Jake Mannix wrote:
> It's done, patch passes tests, but it involves adding 308 lines
> (removing 556 others!) across 84 classes in the codebase, so it probably
> touches something you wrote.
>
> So I'm going to let this diff marinade for a day or so, but if you feel
> like you've got some thoughts (+1's? -1's? I'll even take some +epsilons
> if you're feeling cowardly) on it, feel free to comment on the
> ticket<https://issues.apache.org/jira/browse/MAHOUT-1227> or
> on the review <https://reviews.apache.org/r/11359> itself.
>
> I won't let the patch stick around forever though, as most every commit
> that hits trunk will force me to regenerate this patch again, given its
> breadth.
>
Re: Comments on MAHOUT-1227 ?
Posted by Ted Dunning <te...@gmail.com>.
OK.
Data rules.
If this leads to error, remove it.
I reverse my opinion
On Thu, May 23, 2013 at 7:56 PM, Jake Mannix <ja...@gmail.com> wrote:
> I dunno man, I think we've found 2 errors so far because the dev glibly did
> the simplest possible thing and did for (Element e : vector), and they
> really meant for (Element e : vector.nonZeroes()).
>
> My thinking is this: if you only have one kind of iterator, then it clearly
> makes sense to implement Iterable. If you provide two different iterators,
> neither of which is clearly the right one, then don't implement Iterable,
> as it shows preference for one iterator. In this case, it actually shows
> preference for the *wrong* iterator.
>
>
> On Thu, May 23, 2013 at 7:33 PM, Ted Dunning <te...@gmail.com>
> wrote:
>
> > Looks good to me.
> >
> > I still don't like the all() usage.
> >
> > Can you leave the interface in place on the vector so one has the choice
> of
> > idioms?
> >
> >
> >
> > On Thu, May 23, 2013 at 7:06 PM, Jake Mannix <ja...@gmail.com>
> > wrote:
> >
> > > It's done, patch passes tests, but it involves adding 308 lines
> > > (removing 556 others!) across 84 classes in the codebase, so it
> probably
> > > touches something you wrote.
> > >
> > > So I'm going to let this diff marinade for a day or so, but if you feel
> > > like you've got some thoughts (+1's? -1's? I'll even take some
> > +epsilons
> > > if you're feeling cowardly) on it, feel free to comment on the
> > > ticket<https://issues.apache.org/jira/browse/MAHOUT-1227> or
> > > on the review <https://reviews.apache.org/r/11359> itself.
> > >
> > > I won't let the patch stick around forever though, as most every commit
> > > that hits trunk will force me to regenerate this patch again, given its
> > > breadth.
> > >
> > > --
> > >
> > > -jake
> > >
> >
>
>
>
> --
>
> -jake
>
Re: Comments on MAHOUT-1227 ?
Posted by Jake Mannix <ja...@gmail.com>.
I dunno man, I think we've found 2 errors so far because the dev glibly did
the simplest possible thing and did for (Element e : vector), and they
really meant for (Element e : vector.nonZeroes()).
My thinking is this: if you only have one kind of iterator, then it clearly
makes sense to implement Iterable. If you provide two different iterators,
neither of which is clearly the right one, then don't implement Iterable,
as it shows preference for one iterator. In this case, it actually shows
preference for the *wrong* iterator.
On Thu, May 23, 2013 at 7:33 PM, Ted Dunning <te...@gmail.com> wrote:
> Looks good to me.
>
> I still don't like the all() usage.
>
> Can you leave the interface in place on the vector so one has the choice of
> idioms?
>
>
>
> On Thu, May 23, 2013 at 7:06 PM, Jake Mannix <ja...@gmail.com>
> wrote:
>
> > It's done, patch passes tests, but it involves adding 308 lines
> > (removing 556 others!) across 84 classes in the codebase, so it probably
> > touches something you wrote.
> >
> > So I'm going to let this diff marinade for a day or so, but if you feel
> > like you've got some thoughts (+1's? -1's? I'll even take some
> +epsilons
> > if you're feeling cowardly) on it, feel free to comment on the
> > ticket<https://issues.apache.org/jira/browse/MAHOUT-1227> or
> > on the review <https://reviews.apache.org/r/11359> itself.
> >
> > I won't let the patch stick around forever though, as most every commit
> > that hits trunk will force me to regenerate this patch again, given its
> > breadth.
> >
> > --
> >
> > -jake
> >
>
--
-jake
Re: Comments on MAHOUT-1227 ?
Posted by Ted Dunning <te...@gmail.com>.
Looks good to me.
I still don't like the all() usage.
Can you leave the interface in place on the vector so one has the choice of
idioms?
On Thu, May 23, 2013 at 7:06 PM, Jake Mannix <ja...@gmail.com> wrote:
> It's done, patch passes tests, but it involves adding 308 lines
> (removing 556 others!) across 84 classes in the codebase, so it probably
> touches something you wrote.
>
> So I'm going to let this diff marinade for a day or so, but if you feel
> like you've got some thoughts (+1's? -1's? I'll even take some +epsilons
> if you're feeling cowardly) on it, feel free to comment on the
> ticket<https://issues.apache.org/jira/browse/MAHOUT-1227> or
> on the review <https://reviews.apache.org/r/11359> itself.
>
> I won't let the patch stick around forever though, as most every commit
> that hits trunk will force me to regenerate this patch again, given its
> breadth.
>
> --
>
> -jake
>