You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mahout.apache.org by "Karl Wettin (JIRA)" <ji...@apache.org> on 2008/04/18 21:10:21 UTC
[jira] Commented: (MAHOUT-25) Minor bugs/issues from code
inspection
[ 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.
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