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
