Mahout
  1. Mahout
  2. MAHOUT-67

plus method and divide method in AbstractVector can be optimized for SparseVectors

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.2
    • Component/s: Math
    • Labels:
      None
    1. MAHOUT-67.patch
      2 kB
      Pallavi Palleti
    2. MAHOUT-67.patch
      4 kB
      Pallavi Palleti
    3. MAHOUT-67.patch
      2 kB
      Pallavi Palleti
    4. MAHOUT-67.patch
      2 kB
      Pallavi Palleti

      Activity

      Pallavi Palleti created issue -
      Hide
      Pallavi Palleti added a comment -

      I have overridden plus() method in SparseVector. Inorder to do this, I need to add a method called addCardinality().
      Also, I found that in dot() method, it shouldn't throw cardinalityException. So, I removed that condition. Please review the code.

      Thanks
      Pallavi

      Show
      Pallavi Palleti added a comment - I have overridden plus() method in SparseVector. Inorder to do this, I need to add a method called addCardinality(). Also, I found that in dot() method, it shouldn't throw cardinalityException. So, I removed that condition. Please review the code. Thanks Pallavi
      Pallavi Palleti made changes -
      Field Original Value New Value
      Attachment MAHOUT-67.patch [ 12385479 ]
      Hide
      Isabel Drost-Fromm added a comment -

      Instead of using the following explicitly, you might consider using Java5 style notation, as vector implements Iterable:

      > java.util.Iterator<Vector.Element> iterator = x.iterator();
      > while (iterator.hasNext()) {
      > Vector.Element element = iterator.next();

      I haven't tried it, but it should be possible to use the following expression:

      for (Vector.Element element : x)

      { ... }

      I always find this style easier to read than the explicit use of iterator()

      Isabel

      Show
      Isabel Drost-Fromm added a comment - Instead of using the following explicitly, you might consider using Java5 style notation, as vector implements Iterable: > java.util.Iterator<Vector.Element> iterator = x.iterator(); > while (iterator.hasNext()) { > Vector.Element element = iterator.next(); I haven't tried it, but it should be possible to use the following expression: for (Vector.Element element : x) { ... } I always find this style easier to read than the explicit use of iterator() Isabel
      Hide
      Karl Wettin added a comment - - edited

      You say you need to modify the cardinality. I'm not sure about what has been said about the constraints of Vector modifiers, but the code seems rather static about Vectors being compatible from the start.

      [deleted some stuff here that made no sense]

      Show
      Karl Wettin added a comment - - edited You say you need to modify the cardinality. I'm not sure about what has been said about the constraints of Vector modifiers, but the code seems rather static about Vectors being compatible from the start. [deleted some stuff here that made no sense]
      Hide
      Pallavi Palleti added a comment -

      Isabel: Sure, I will make those changes.

      Karl: I couldn't get. Can you please elaborate it?

      Show
      Pallavi Palleti added a comment - Isabel: Sure, I will make those changes. Karl: I couldn't get. Can you please elaborate it?
      Hide
      Pallavi Palleti added a comment -

      also added divide method

      Show
      Pallavi Palleti added a comment - also added divide method
      Pallavi Palleti made changes -
      Summary plus method in AbstractVector doesn't work for SparseVectors plus method and divide method in AbstractVector doesn't work for SparseVectors
      Hide
      Pallavi Palleti added a comment -

      changed to Java5 notation style.
      Also added divide method for sparse vector.

      Show
      Pallavi Palleti added a comment - changed to Java5 notation style. Also added divide method for sparse vector.
      Pallavi Palleti made changes -
      Attachment MAHOUT-67.patch [ 12385602 ]
      Hide
      Isabel Drost-Fromm added a comment -

      Could you please also add a unit test that shows, that the current implementation is broken but your version works?

      Show
      Isabel Drost-Fromm added a comment - Could you please also add a unit test that shows, that the current implementation is broken but your version works?
      Hide
      Pallavi Palleti added a comment -

      I have added unit tests to show that the current implementation breaks for sparse vector plus, dot and divide operation. and my version works.

      Show
      Pallavi Palleti added a comment - I have added unit tests to show that the current implementation breaks for sparse vector plus, dot and divide operation. and my version works.
      Pallavi Palleti made changes -
      Attachment MAHOUT-67.patch [ 12387618 ]
      Hide
      Pallavi Palleti added a comment -

      I misunderstood the sparse vector representation. I agree that the cardinality exception should be thrown if two sparse vectors' cardinality is not same. But, my implementation still holds and optimizes the computation of divide and plus operation over sparse vectors.

      Show
      Pallavi Palleti added a comment - I misunderstood the sparse vector representation. I agree that the cardinality exception should be thrown if two sparse vectors' cardinality is not same. But, my implementation still holds and optimizes the computation of divide and plus operation over sparse vectors.
      Pallavi Palleti made changes -
      Summary plus method and divide method in AbstractVector doesn't work for SparseVectors plus method and divide method in AbstractVector can be optimized for SparseVectors
      Hide
      Pallavi Palleti added a comment -

      I have restored the cardinality check in dot method and also added cardinality check for plus method and so updated unit tests accordingly. As this code is optimization of existing code, the previous unit tests holds.

      Show
      Pallavi Palleti added a comment - I have restored the cardinality check in dot method and also added cardinality check for plus method and so updated unit tests accordingly. As this code is optimization of existing code, the previous unit tests holds.
      Pallavi Palleti made changes -
      Attachment MAHOUT-67.patch [ 12387623 ]
      Grant Ingersoll made changes -
      Workflow jira [ 12434834 ] no-reopen-closed, patch-avail [ 12444698 ]
      Hide
      Sean Owen added a comment -

      I checked, and at this point AbstractVector already does what this patch did, through use of iterateNonZero(). Marking this basically obsoleted.

      Show
      Sean Owen added a comment - I checked, and at this point AbstractVector already does what this patch did, through use of iterateNonZero(). Marking this basically obsoleted.
      Sean Owen made changes -
      Status Open [ 1 ] Resolved [ 5 ]
      Assignee Sean Owen [ srowen ]
      Fix Version/s 0.2 [ 12313278 ]
      Resolution Fixed [ 1 ]
      Sean Owen made changes -
      Status Resolved [ 5 ] Closed [ 6 ]
      Transition Time In Source Status Execution Times Last Executer Last Execution Date
      Open Open Resolved Resolved
      516d 6h 24m 1 Sean Owen 06/Dec/09 16:15
      Resolved Resolved Closed Closed
      530d 11h 8m 1 Sean Owen 21/May/11 04:24

        People

        • Assignee:
          Sean Owen
          Reporter:
          Pallavi Palleti
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development