You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mahout.apache.org by "Sean Owen (JIRA)" <ji...@apache.org> on 2008/04/05 05:01:24 UTC
[jira] Created: (MAHOUT-25) Minor bugs/issues from code inspection
Minor bugs/issues from code inspection
--------------------------------------
Key: MAHOUT-25
URL: https://issues.apache.org/jira/browse/MAHOUT-25
Project: Mahout
Issue Type: Bug
Affects Versions: 0.1
Environment: All
Reporter: Sean Owen
Priority: Minor
Fix For: 0.1
Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
MatrixView:57
for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
Pretty sure that should be col++ at the end.
DenseMatrix:122
Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
Canopy:146, 168
pointStronglyBound = pointStronglyBound | (dist < t2);
Should this really be a non-shortcircuit, versus
pointStronglyBound = pointStronglyBound || (dist < t2);
or just
if (!pointStronglyBound) {
pointStronglyBound = dist < t2;
}
CanopyDriver:52,53
String.valueOf(x) is a smidge faster than "" + x.
Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Created: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by Jeff Eastman <je...@windwardsolutions.com>.
Sean Owen (JIRA) wrote:
> Minor bugs/issues from code inspection
> --------------------------------------
>
> Key: MAHOUT-25
> URL: https://issues.apache.org/jira/browse/MAHOUT-25
> Project: Mahout
> Issue Type: Bug
> Affects Versions: 0.1
> Environment: All
> Reporter: Sean Owen
> Priority: Minor
> Fix For: 0.1
>
>
> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
>
>
> MatrixView:57
>
> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
>
> Pretty sure that should be col++ at the end.
>
>
> DenseMatrix:122
>
> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
>
>
> Canopy:146, 168
>
> pointStronglyBound = pointStronglyBound | (dist < t2);
>
> Should this really be a non-shortcircuit, versus
>
> pointStronglyBound = pointStronglyBound || (dist < t2);
>
> or just
>
> if (!pointStronglyBound) {
> pointStronglyBound = dist < t2;
> }
>
>
> CanopyDriver:52,53
>
> String.valueOf(x) is a smidge faster than "" + x.
>
>
> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
>
>
>
Indeed, the MatrixView class has some problems as Ted noted in
MAHOUT-23. I posted a patch to that issue with one of the ones you
found. I'm waiting for credentials to commit that patch. I will also
take a look at the others you have noted.
Thanks,
Jeff
Re: [jira] Commented: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by Jeff Eastman <je...@windwardsolutions.com>.
Karl Wettin (JIRA) wrote:
> [ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12590577#action_12590577 ]
>
> Karl Wettin commented on MAHOUT-25:
> -----------------------------------
>
> +1, patch looks good.
>
> Somewhat related, I wish AbstractMatrix.ROW and COL was an enumeration instead so it could be import it to other classes where you then only have to type ROW rather than AbstractMatrix.ROW.
>
> But perhaps that would force me to type ROW.intValue() instead. Think I still prefere that.
>
>
>> Minor bugs/issues from code inspection
>> --------------------------------------
>>
>> Key: MAHOUT-25
>> URL: https://issues.apache.org/jira/browse/MAHOUT-25
>> Project: Mahout
>> Issue Type: Bug
>> Components: Matrix
>> Affects Versions: 0.1
>> Environment: All
>> Reporter: Sean Owen
>> Priority: Minor
>> Fix For: 0.1
>>
>> Attachments: MAHOUT-25.patch
>>
>>
>> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
>> MatrixView:57
>> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
>> Pretty sure that should be col++ at the end.
>> DenseMatrix:122
>> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
>> Canopy:146, 168
>> pointStronglyBound = pointStronglyBound | (dist < t2);
>> Should this really be a non-shortcircuit, versus
>> pointStronglyBound = pointStronglyBound || (dist < t2);
>> or just
>> if (!pointStronglyBound) {
>> pointStronglyBound = dist < t2;
>> }
>> CanopyDriver:52,53
>> String.valueOf(x) is a smidge faster than "" + x.
>> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
>>
>
>
+1 on this and the other changes too. I just committed a test for
MatrixView and some MatrixView changes from MAHOUT-23 that may have
corrected one problem you found, so update first as there may be one
conflict.
Jeff
[jira] Commented: (MAHOUT-25) Minor bugs/issues from code
inspection
Posted by "Karl Wettin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12590577#action_12590577 ]
Karl Wettin commented on MAHOUT-25:
-----------------------------------
+1, patch looks good.
Somewhat related, I wish AbstractMatrix.ROW and COL was an enumeration instead so it could be import it to other classes where you then only have to type ROW rather than AbstractMatrix.ROW.
But perhaps that would force me to type ROW.intValue() instead. Think I still prefere that.
> Minor bugs/issues from code inspection
> --------------------------------------
>
> Key: MAHOUT-25
> URL: https://issues.apache.org/jira/browse/MAHOUT-25
> Project: Mahout
> Issue Type: Bug
> Components: Matrix
> Affects Versions: 0.1
> Environment: All
> Reporter: Sean Owen
> Priority: Minor
> Fix For: 0.1
>
> Attachments: MAHOUT-25.patch
>
>
> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
> MatrixView:57
> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
> Pretty sure that should be col++ at the end.
> DenseMatrix:122
> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
> Canopy:146, 168
> pointStronglyBound = pointStronglyBound | (dist < t2);
> Should this really be a non-shortcircuit, versus
> pointStronglyBound = pointStronglyBound || (dist < t2);
> or just
> if (!pointStronglyBound) {
> pointStronglyBound = dist < t2;
> }
> CanopyDriver:52,53
> String.valueOf(x) is a smidge faster than "" + x.
> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by "Jeff Eastman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jeff Eastman updated MAHOUT-25:
-------------------------------
Component/s: Matrix
> Minor bugs/issues from code inspection
> --------------------------------------
>
> Key: MAHOUT-25
> URL: https://issues.apache.org/jira/browse/MAHOUT-25
> Project: Mahout
> Issue Type: Bug
> Components: Matrix
> Affects Versions: 0.1
> Environment: All
> Reporter: Sean Owen
> Priority: Minor
> Fix For: 0.1
>
>
> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
> MatrixView:57
> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
> Pretty sure that should be col++ at the end.
> DenseMatrix:122
> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
> Canopy:146, 168
> pointStronglyBound = pointStronglyBound | (dist < t2);
> Should this really be a non-shortcircuit, versus
> pointStronglyBound = pointStronglyBound || (dist < t2);
> or just
> if (!pointStronglyBound) {
> pointStronglyBound = dist < t2;
> }
> CanopyDriver:52,53
> String.valueOf(x) is a smidge faster than "" + x.
> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Updated: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by Sean Owen <sr...@gmail.com>.
Good view, and, likewise I'm open to people changing the code coming
in the door from me.
I want to start being helpful and this was the first thing I saw I
could do, but, want to acknowledge that it could be viewed as
obnoxious. I won't send around changelists like that regularly.
On Fri, Apr 18, 2008 at 2:56 PM, Grant Ingersoll <gs...@apache.org> wrote:
> Oh, but it is your code! Haven't you received your ASF Indoctrination yet?
> ;-)
Re: [jira] Updated: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by Grant Ingersoll <gs...@apache.org>.
On Apr 18, 2008, at 2:29 PM, Sean Owen wrote:
> OK will wait a beat to let anyone take a look and object. I have tried
> to stay away from anything that might be a matter of taste,
Understood. Was more making the point for everyone, since we have a
number of new-to-the-ASF people in this group
> and more
> like things that might simplify, clarify, optimize or standardize. I
> also want be deferent here since it's not my code;
Oh, but it is your code! Haven't you received your ASF Indoctrination
yet? ;-)
Cheers,
Grant
Re: [jira] Updated: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by Sean Owen <sr...@gmail.com>.
OK will wait a beat to let anyone take a look and object. I have tried
to stay away from anything that might be a matter of taste, and more
like things that might simplify, clarify, optimize or standardize. I
also want be deferent here since it's not my code; just possibly
helpful suggestions from me that are not necessary.
If nobody objects I will commit in a couple days.
On Fri, Apr 18, 2008 at 1:32 PM, Grant Ingersoll <gs...@apache.org> wrote:
> Just FYI, silence in Apache-land almost always (like 99% of the time) means
> agreement. Basically, it means you're a committer and we trust you to do
> the right thing!
>
> -Grant
>
>
>
> On Apr 17, 2008, at 9:17 PM, Sean Owen (JIRA) wrote:
>
>
> >
> > [
> https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
> ]
> >
> > Sean Owen updated MAHOUT-25:
> > ----------------------------
> >
> > Attachment: MAHOUT-25.patch
> >
> > OK I'll only try this once. Here's a collection of small tweaks to the
> code that I would suggest. Everything compiles and unit tests pass. I am
> happy to be talked out of this, but thought I would offer it, as it does
> standardize the code a bit.
> >
> >
> > > Minor bugs/issues from code inspection
> > > --------------------------------------
> > >
> > > Key: MAHOUT-25
> > > URL: https://issues.apache.org/jira/browse/MAHOUT-25
> > > Project: Mahout
> > > Issue Type: Bug
> > > Components: Matrix
> > > Affects Versions: 0.1
> > > Environment: All
> > > Reporter: Sean Owen
> > > Priority: Minor
> > > Fix For: 0.1
> > >
> > > Attachments: MAHOUT-25.patch
> > >
> > >
> > > Hi all, just started checking out the code base to get familiar with it
> and turned loose IntelliJ on the code. It picked up a few possible issues I
> thought I'd highlight:
> > > MatrixView:57
> > > for (int col = offset[COL]; col < offset[COL] + cardinality[COL];
> row++)
> > > Pretty sure that should be col++ at the end.
> > > DenseMatrix:122
> > > Instances are compared uisng == instead of equals(). Doesn't matter if
> this is exactly the semantics you want, but if DenseMatrix ever defined a
> notion of equals() this wouldn't use it and might be a bug. Same in many
> other classes.
> > > Canopy:146, 168
> > > pointStronglyBound = pointStronglyBound | (dist < t2);
> > > Should this really be a non-shortcircuit, versus
> > > pointStronglyBound = pointStronglyBound || (dist < t2);
> > > or just
> > > if (!pointStronglyBound) {
> > > pointStronglyBound = dist < t2;
> > > }
> > > CanopyDriver:52,53
> > > String.valueOf(x) is a smidge faster than "" + x.
> > > Actually I've got several more but they're less important, like
> redundant casts or utility classes without private constructors, etc. I can
> keep going here... want to help polish a few things but don't want to get
> overbearing.
> > >
> >
> > --
> > This message is automatically generated by JIRA.
> > -
> > You can reply to this email to add a comment to the issue online.
> >
> >
>
> --------------------------
> Grant Ingersoll
>
> Lucene Helpful Hints:
> http://wiki.apache.org/lucene-java/BasicsOfPerformance
> http://wiki.apache.org/lucene-java/LuceneFAQ
>
>
>
>
>
>
>
Re: [jira] Updated: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by Grant Ingersoll <gs...@apache.org>.
Just FYI, silence in Apache-land almost always (like 99% of the time)
means agreement. Basically, it means you're a committer and we trust
you to do the right thing!
-Grant
On Apr 17, 2008, at 9:17 PM, Sean Owen (JIRA) wrote:
>
> [ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
> ]
>
> Sean Owen updated MAHOUT-25:
> ----------------------------
>
> Attachment: MAHOUT-25.patch
>
> OK I'll only try this once. Here's a collection of small tweaks to
> the code that I would suggest. Everything compiles and unit tests
> pass. I am happy to be talked out of this, but thought I would offer
> it, as it does standardize the code a bit.
>
>> Minor bugs/issues from code inspection
>> --------------------------------------
>>
>> Key: MAHOUT-25
>> URL: https://issues.apache.org/jira/browse/MAHOUT-25
>> Project: Mahout
>> Issue Type: Bug
>> Components: Matrix
>> Affects Versions: 0.1
>> Environment: All
>> Reporter: Sean Owen
>> Priority: Minor
>> Fix For: 0.1
>>
>> Attachments: MAHOUT-25.patch
>>
>>
>> Hi all, just started checking out the code base to get familiar
>> with it and turned loose IntelliJ on the code. It picked up a few
>> possible issues I thought I'd highlight:
>> MatrixView:57
>> for (int col = offset[COL]; col < offset[COL] +
>> cardinality[COL]; row++)
>> Pretty sure that should be col++ at the end.
>> DenseMatrix:122
>> Instances are compared uisng == instead of equals(). Doesn't matter
>> if this is exactly the semantics you want, but if DenseMatrix ever
>> defined a notion of equals() this wouldn't use it and might be a
>> bug. Same in many other classes.
>> Canopy:146, 168
>> pointStronglyBound = pointStronglyBound | (dist < t2);
>> Should this really be a non-shortcircuit, versus
>> pointStronglyBound = pointStronglyBound || (dist < t2);
>> or just
>> if (!pointStronglyBound) {
>> pointStronglyBound = dist < t2;
>> }
>> CanopyDriver:52,53
>> String.valueOf(x) is a smidge faster than "" + x.
>> Actually I've got several more but they're less important, like
>> redundant casts or utility classes without private constructors,
>> etc. I can keep going here... want to help polish a few things but
>> don't want to get overbearing.
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
--------------------------
Grant Ingersoll
Lucene Helpful Hints:
http://wiki.apache.org/lucene-java/BasicsOfPerformance
http://wiki.apache.org/lucene-java/LuceneFAQ
[jira] Updated: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by "Sean Owen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sean Owen updated MAHOUT-25:
----------------------------
Attachment: MAHOUT-25.patch
OK I'll only try this once. Here's a collection of small tweaks to the code that I would suggest. Everything compiles and unit tests pass. I am happy to be talked out of this, but thought I would offer it, as it does standardize the code a bit.
> Minor bugs/issues from code inspection
> --------------------------------------
>
> Key: MAHOUT-25
> URL: https://issues.apache.org/jira/browse/MAHOUT-25
> Project: Mahout
> Issue Type: Bug
> Components: Matrix
> Affects Versions: 0.1
> Environment: All
> Reporter: Sean Owen
> Priority: Minor
> Fix For: 0.1
>
> Attachments: MAHOUT-25.patch
>
>
> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
> MatrixView:57
> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
> Pretty sure that should be col++ at the end.
> DenseMatrix:122
> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
> Canopy:146, 168
> pointStronglyBound = pointStronglyBound | (dist < t2);
> Should this really be a non-shortcircuit, versus
> pointStronglyBound = pointStronglyBound || (dist < t2);
> or just
> if (!pointStronglyBound) {
> pointStronglyBound = dist < t2;
> }
> CanopyDriver:52,53
> String.valueOf(x) is a smidge faster than "" + x.
> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (MAHOUT-25) Minor bugs/issues from code inspection
Posted by "Sean Owen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sean Owen resolved MAHOUT-25.
-----------------------------
Resolution: Fixed
(Somebody holler if I am not following protocol)
I committed my changes for this issue and am closing it.
> Minor bugs/issues from code inspection
> --------------------------------------
>
> Key: MAHOUT-25
> URL: https://issues.apache.org/jira/browse/MAHOUT-25
> Project: Mahout
> Issue Type: Bug
> Components: Matrix
> Affects Versions: 0.1
> Environment: All
> Reporter: Sean Owen
> Priority: Minor
> Fix For: 0.1
>
> Attachments: MAHOUT-25.patch
>
>
> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
> MatrixView:57
> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
> Pretty sure that should be col++ at the end.
> DenseMatrix:122
> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
> Canopy:146, 168
> pointStronglyBound = pointStronglyBound | (dist < t2);
> Should this really be a non-shortcircuit, versus
> pointStronglyBound = pointStronglyBound || (dist < t2);
> or just
> if (!pointStronglyBound) {
> pointStronglyBound = dist < t2;
> }
> CanopyDriver:52,53
> String.valueOf(x) is a smidge faster than "" + x.
> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (MAHOUT-25) Minor bugs/issues from code
inspection
Posted by "Karl Wettin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-25?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12590660#action_12590660 ]
Karl Wettin commented on MAHOUT-25:
-----------------------------------
bq. (Somebody holler if I am not following protocol)
You could just hit close and skip the resolve step. It's used in for instance in organisations where a tester reports a bug, coder fix and hit resolve, tester confirms it's all good and close the issue.
> Minor bugs/issues from code inspection
> --------------------------------------
>
> Key: MAHOUT-25
> URL: https://issues.apache.org/jira/browse/MAHOUT-25
> Project: Mahout
> Issue Type: Bug
> Components: Matrix
> Affects Versions: 0.1
> Environment: All
> Reporter: Sean Owen
> Priority: Minor
> Fix For: 0.1
>
> Attachments: MAHOUT-25.patch
>
>
> Hi all, just started checking out the code base to get familiar with it and turned loose IntelliJ on the code. It picked up a few possible issues I thought I'd highlight:
> MatrixView:57
> for (int col = offset[COL]; col < offset[COL] + cardinality[COL]; row++)
> Pretty sure that should be col++ at the end.
> DenseMatrix:122
> Instances are compared uisng == instead of equals(). Doesn't matter if this is exactly the semantics you want, but if DenseMatrix ever defined a notion of equals() this wouldn't use it and might be a bug. Same in many other classes.
> Canopy:146, 168
> pointStronglyBound = pointStronglyBound | (dist < t2);
> Should this really be a non-shortcircuit, versus
> pointStronglyBound = pointStronglyBound || (dist < t2);
> or just
> if (!pointStronglyBound) {
> pointStronglyBound = dist < t2;
> }
> CanopyDriver:52,53
> String.valueOf(x) is a smidge faster than "" + x.
> Actually I've got several more but they're less important, like redundant casts or utility classes without private constructors, etc. I can keep going here... want to help polish a few things but don't want to get overbearing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.