commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Barker" <billwbar...@verizon.net>
Subject Re: svn commit: r889008 - in /commons/proper/math/trunk/src: main/java/org/apache/commons/math/linear/ site/xdoc/ test/java/org/apache/commons/math/linear/
Date Thu, 10 Dec 2009 05:28:20 GMT
There are some things I don't like about this, but will try to find the time 
to fix them myself .  Comments inline (and I can't veto anything in [math], 
so these are just suggestions).

----- Original Message ----- 
From: <luc@apache.org>
To: <commits@commons.apache.org>
Sent: Wednesday, December 09, 2009 2:56 PM
Subject: svn commit: r889008 - in /commons/proper/math/trunk/src: 
main/java/org/apache/commons/math/linear/ site/xdoc/ 
test/java/org/apache/commons/math/linear/


> Author: luc
> Date: Wed Dec  9 22:56:11 2009
> New Revision: 889008
>
> URL: http://svn.apache.org/viewvc?rev=889008&view=rev
> Log:
> Added mapping and iteration methods to vectors.
> Provided a default implementation for the numerous simple methods in the 
> RealVectorInterface.
>
> +    protected class SparseEntryIterator implements Iterator<Entry> {
> +
> +        /** Dimension of the vector. */
> +        private final int dim;
> +
> +        /** Temporary entry (reused on each call to {@link #next()}. */
> +        private EntryImpl tmp = new EntryImpl();
> +
> +        /** Current entry. */
> +        private EntryImpl current;
> +
> +        /** Next entry. */
> +        private EntryImpl next;
> +
> +        /** Simple constructor. */
> +        protected SparseEntryIterator() {
> +            dim = getDimension();
> +            current = new EntryImpl();
> +            if (current.getValue() == 0) {
> +                advance(current);
> +            }

This totally doesn't work if the vector consists of all zero elements.  The 
'hasNext' method (below) will return true, and you will get an exeption 
trying to get the first element.

> +            next = new EntryImpl();
> +            next.setIndex(current.getIndex());
> +            advance(next);
> +        }
> +
> +        /** Advance an entry up to the next non null one.
> +         * @param e entry to advance
> +         */
> +        protected void advance(EntryImpl e) {
> +            if (e == null) {
> +                return;
> +            }
> +            do {
> +                e.setIndex(e.getIndex() + 1);
> +            } while (e.getIndex() < dim && e.getValue() == 0);
> +            if (e.getIndex() >= dim) {
> +                e.setIndex(-1);
> +            }
> +        }

This is fragile, since it relies on e.getIndex() == -1 when invalid (and 
then you scan the vector once more).

> +
> +        /** {@inheritDoc} */
> +        public boolean hasNext() {
> +            return current != null;
> +        }
> +

quick fix is to check that current.getIndex() >= 0

> +        /** {@inheritDoc} */
> +        public Entry next() {
> +            tmp.setIndex(current.getIndex());
> +            if (next != null) {
> +                current.setIndex(next.getIndex());
> +                advance(next);
> +                if (next.getIndex() < 0) {
> +                    next = null;
> +                }
> +            } else {
> +                current = null;
> +            }
> +            return tmp;
> +        }
> +
> +        /** {@inheritDoc} */
> +        public void remove() {
> +            throw new UnsupportedOperationException("Not supported");
> +        }
> +    }
> +
> +}
>
> Modified: 
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/linear/OpenMapRealVector.java
> URL: 
> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/linear/OpenMapRealVector.java?rev=889008&r1=889007&r2=889008&view=diff
> ==============================================================================
> ---  
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/linear/OpenMapRealVector.java

> (original)
> +++ 
> commons/proper/math/trunk/src/main/java/org/apache/commons/math/linear/OpenMapRealVector.java

> Wed Dec  9 22:56:11 2009
> @@ -27,7 +27,7 @@
>  * @version $Revision$ $Date$
>  * @since 2.0
> */
> -public class OpenMapRealVector implements SparseRealVector, Serializable 
> {
> +public class OpenMapRealVector extends AbstractRealVector implements 
> SparseRealVector, Serializable {
>
>     /** Default Tolerance for having a value considered zero. */
>     public static final double DEFAULT_ZERO_TOLERANCE = 1.0e-12;
> @@ -41,8 +41,11 @@
>     /** Dimension of the vector. */
>     private final int virtualSize;
>
> -    /** Tolerance for having a value considered zero. */
> -    private double epsilon;
> +    /** Negative tolerance for having a value considered zero. */
> +    private double minusEpsilon;
> +
> +    /** Positive tolerance for having a value considered zero. */
> +    private double plusEpsilon;
>

If non-zero defaultValues are allowed, then we need to store the 
defaultValue, to avoid floating point precision issues where (plusEpsilon + 
minusEpsilon)/2 != defaultValue.

>     /**
>      * Build a 0-length vector.
> @@ -54,7 +57,7 @@
>      * into this vector.</p>
>      */
>     public OpenMapRealVector() {
> -        this(0, DEFAULT_ZERO_TOLERANCE);
> +        this(0, DEFAULT_ZERO_TOLERANCE, 0);
>     }
>
>     /**
> @@ -62,18 +65,19 @@
>      * @param dimension size of the vector
>      */
>     public OpenMapRealVector(int dimension) {
> -        this(dimension, DEFAULT_ZERO_TOLERANCE);
> +        this(dimension, DEFAULT_ZERO_TOLERANCE, 0);
>     }
>
>     /**
>      * Construct a (dimension)-length vector of zeros, specifying zero 
> tolerance.
>      * @param dimension Size of the vector
>      * @param epsilon The tolerance for having a value considered zero
> +     * @param defaultValue value for non-specified entries
>      */
> -    public OpenMapRealVector(int dimension, double epsilon) {
> +    public OpenMapRealVector(int dimension, double epsilon, double 
> defaultValue) {
>         virtualSize = dimension;
> -        entries = new OpenIntToDoubleHashMap(0.0);
> -        this.epsilon = epsilon;
> +        entries = new OpenIntToDoubleHashMap(defaultValue);
> +        setDefault(defaultValue, epsilon);
>     }
>

As I stated in the Jira issue, I really dislike supporting non-zero 
defaultValues here.  It causes too many things to just produce nonsensical 
results:

   RealVector v = new OpenMapRealVector(1000, 1.0e-12, 1);
   assertEquals(v.getEntry(0), 0); // passes

   RealVector v = new OpenMapRealVector(1000, 1.0e-12, 1);
   RealVector w = new OpenMapRealVector(1000, 1.0e-12, 2);
   assertEquals(v.add(w).getEntry(0), 0); // passes



>     /** {@inheritDoc} */
> -    public OpenMapRealVector add(RealVector v) throws 
> IllegalArgumentException {
> +    public RealVector add(RealVector v) throws IllegalArgumentException {
>         checkVectorDimensions(v.getDimension());
>         if (v instanceof OpenMapRealVector) {
>             return add((OpenMapRealVector) v);
> +        } else {
> +            return super.add(v);
>         }
> -        return add(v.getData());
>     }
>
>     /**
> -     * Optimized method to add two OpenMapRealVectors.
> +     * Optimized method to add two OpenMapRealVectors.  Copies the larger 
> vector, iterates over the smaller.
>      * @param v Vector to add with
>      * @return The sum of <code>this</code> with <code>v</code>
>      * @throws IllegalArgumentException If the dimensions don't match
>      */
>     public OpenMapRealVector add(OpenMapRealVector v) throws 
> IllegalArgumentException{
>         checkVectorDimensions(v.getDimension());
> -        OpenMapRealVector res = copy();
> -        Iterator iter = v.getEntries().iterator();
> +        boolean copyThis = entries.size() > v.entries.size();
> +        OpenMapRealVector res = copyThis ? this.copy() : v.copy();
> +        Iterator iter = copyThis ? v.entries.iterator() : 
> entries.iterator();
> +        OpenIntToDoubleHashMap randomAccess = copyThis ? entries : 
> v.entries;
>         while (iter.hasNext()) {
>             iter.advance();
>             int key = iter.key();
> -            if (entries.containsKey(key)) {
> -                res.setEntry(key, entries.get(key) + iter.value());
> +            if (randomAccess.containsKey(key)) {
> +                res.setEntry(key, randomAccess.get(key) + iter.value());
>             } else {
>                 res.setEntry(key, iter.value());
>             }

This only works (in a wonderland sense of working) when both sides have the 
same defaultValue.

> @@ -252,16 +259,6 @@
>         return res;
>     }
>
> -    /** {@inheritDoc} */
> -    public double dotProduct(RealVector v) throws 
> IllegalArgumentException {
> -        checkVectorDimensions(v.getDimension());
> -        double res = 0;
> -        Iterator iter = entries.iterator();
> -        while (iter.hasNext()) {
> -            iter.advance();
> -            res += v.getEntry(iter.key()) * iter.value();
> -        }
> -        return res;
> -    }
> -

For this class to be useful, it needs it's own sparseIterator() method to 
cover delegating this method to the base class.

> @@ -1185,7 +1155,7 @@
>     /** {@inheritDoc} */
>     public void unitize() {
>         double norm = getNorm();
> -        if (isZero(norm)) {
> +        if (isDefaultValue(norm)) {
>             throw  MathRuntimeException.createArithmeticException("cannot 
> normalize a zero norm vector");
>         }
>         Iterator iter = entries.iterator();
> @@ -1196,37 +1166,6 @@
>
>     }

This one is just silly.  This results in:
     RealVector v = new OpenMapRealVector(1000, 1.0e-12, 1);
     v.setEntry(0,1);
     v.unitize();
throwing an execption.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message