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 <[EMAIL PROTECTED]> 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 > > > > > > >
