Commons Math
  1. Commons Math
  2. MATH-803

Bugs in RealVector.ebeMultiply(RealVector) and ebeDivide(RealVector)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0
    • Fix Version/s: 3.3
    • Labels:
      None

      Description

      OpenMapRealVector.ebeMultiply(RealVector) and OpenMapRealVector.ebeDivide(RealVector) return wrong values when one entry of the specified RealVector is nan or infinity. The bug is easy to understand. Here is the current implementation of ebeMultiply

          public OpenMapRealVector ebeMultiply(RealVector v) {
              checkVectorDimensions(v.getDimension());
              OpenMapRealVector res = new OpenMapRealVector(this);
              Iterator iter = entries.iterator();
              while (iter.hasNext()) {
                  iter.advance();
                  res.setEntry(iter.key(), iter.value() * v.getEntry(iter.key()));
              }
              return res;
          }
      

      The assumption is that for any double x, x * 0d == 0d holds, which is not true. The bug is easy enough to identify, but more complex to solve. The only solution I can come up with is to loop through all entries of v (instead of those entries which correspond to non-zero entries of this). I'm afraid about performance losses.

        Activity

        Hide
        Sébastien Brisard added a comment -

        In r1348485, RealVectorAbstractTest include unit tests illustrating this bug.

        Show
        Sébastien Brisard added a comment - In r1348485 , RealVectorAbstractTest include unit tests illustrating this bug.
        Hide
        Luc Maisonobe added a comment -

        Perhaps we should use a post-processing branch to handle infinite or spacial values:

        // current implementation looping over non-zero instance elements goes here
        
        if (v.isNaN() || v.isInfinite()) {
          // post-processing loop, to handle 0 * infinity and 0 * NaN cases
          for (int i = 0; i < v.getDimension(); ++v) {
            if (Double.isInfinite(v.getElement(i)) || Double.isNaN(v.getElement(i)) {
               res.setEntry(i, Double.NaN);
            }
          }
        }
        

        This could be fast only if isNaN() and isInfinite() results are cached and the same vector is reused, otherwise the outer if statement should be removed and the post-processing should be done in all cases.

        Show
        Luc Maisonobe added a comment - Perhaps we should use a post-processing branch to handle infinite or spacial values: // current implementation looping over non-zero instance elements goes here if (v.isNaN() || v.isInfinite()) { // post-processing loop, to handle 0 * infinity and 0 * NaN cases for ( int i = 0; i < v.getDimension(); ++v) { if ( Double .isInfinite(v.getElement(i)) || Double .isNaN(v.getElement(i)) { res.setEntry(i, Double .NaN); } } } This could be fast only if isNaN() and isInfinite() results are cached and the same vector is reused, otherwise the outer if statement should be removed and the post-processing should be done in all cases.
        Hide
        Sébastien Brisard added a comment -

        I'm going to implement the suggested post-processing for the time being; this should solve the bug. Caching of isNaN() and isInfinite() is postponed.

        Show
        Sébastien Brisard added a comment - I'm going to implement the suggested post-processing for the time being; this should solve the bug. Caching of isNaN() and isInfinite() is postponed.
        Hide
        Sébastien Brisard added a comment -

        I think this does not work for ebeDivide(RealVector). In this case, I suggest to revert to naive implementation (loop through all entries).

        Show
        Sébastien Brisard added a comment - I think this does not work for ebeDivide(RealVector) . In this case, I suggest to revert to naive implementation (loop through all entries).
        Hide
        Sébastien Brisard added a comment -

        Changed the title of the ticket, since the scope of this bug is much broader. Indeed, it affects RealVector.ebeMultiply(RealVector) and RealVector.ebeDivide(RealVector) as soon as the RealVector passed as a parameter is sparse, see examples below

        1. for ebeMultiply()
          final RealVector v1 = new ArrayRealVector(new double[] { 1d });
          final RealVector v2 = new OpenMapRealVector(new double[] { -0d });
          final RealVector w = v1.ebeMultiply(v2);
          System.out.println(1d / w.getEntry(0));
          

          prints Infinity, instead of -Infinity (because the sign is lost in v2). This means that w holds +0d instead of -0d.

        2. for ebeDivide()
          final RealVector v1 = new ArrayRealVector(new double[] { 1d });
          final RealVector v2 = new OpenMapRealVector(new double[] { -0d });
          final RealVector w = v1.ebeDivide(v2);
          System.out.println(w.getEntry(0));
          

          prints Infinity, instead of -Infinity.

        Show
        Sébastien Brisard added a comment - Changed the title of the ticket, since the scope of this bug is much broader. Indeed, it affects RealVector.ebeMultiply(RealVector) and RealVector.ebeDivide(RealVector) as soon as the RealVector passed as a parameter is sparse, see examples below for ebeMultiply() final RealVector v1 = new ArrayRealVector( new double [] { 1d }); final RealVector v2 = new OpenMapRealVector( new double [] { -0d }); final RealVector w = v1.ebeMultiply(v2); System .out.println(1d / w.getEntry(0)); prints Infinity , instead of -Infinity (because the sign is lost in v2 ). This means that w holds +0d instead of -0d . for ebeDivide() final RealVector v1 = new ArrayRealVector( new double [] { 1d }); final RealVector v2 = new OpenMapRealVector( new double [] { -0d }); final RealVector w = v1.ebeDivide(v2); System .out.println(w.getEntry(0)); prints Infinity , instead of -Infinity .
        Hide
        Sébastien Brisard added a comment -

        According to this thread, ebeMultiply and ebeDivide are deprecated.

        Show
        Sébastien Brisard added a comment - According to this thread , ebeMultiply and ebeDivide are deprecated.
        Hide
        Sébastien Brisard added a comment -

        Methods are deprecated in r1352782, and unit tests are skipped (they are kept for reference). Issue is to remain open until the methods are removed in version 4.0.

        Show
        Sébastien Brisard added a comment - Methods are deprecated in r1352782 , and unit tests are skipped (they are kept for reference). Issue is to remain open until the methods are removed in version 4.0.
        Hide
        Luc Maisonobe added a comment -

        Methods undeprecated in r1570246.
        The test cases have been adapted, and the fact we force 0 * x = 0 even for NaNs and infinities is considered acceptable now, as it is similar to standard practive for sparse linear algebra libraries.

        Show
        Luc Maisonobe added a comment - Methods undeprecated in r1570246. The test cases have been adapted, and the fact we force 0 * x = 0 even for NaNs and infinities is considered acceptable now, as it is similar to standard practive for sparse linear algebra libraries.
        Hide
        Luc Maisonobe added a comment -

        Closing all resolved issue now available in released 3.3 version.

        Show
        Luc Maisonobe added a comment - Closing all resolved issue now available in released 3.3 version.

          People

          • Assignee:
            Unassigned
            Reporter:
            Sébastien Brisard
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development