Notes inline...

On 11/24/2015 08:28 AM, Gilles wrote:
On Tue, 24 Nov 2015 06:52:04 -0700, Phil Steitz wrote:
I need the following methods to complete the fix for MATH-1246.  I
can add them as private methods to the KS class; but they seem
generally useful, so I propose adding them to MathArrays.  Any
objections?

/**
  * Concatenates two arrays.
  *
  * @param x first array
  * @param y second array
  * @return a new array consisting of the entries of x followed by
the entries of y
  * @throws NullPointerException if either x or y is null
  */
public static double[] concat(double[] x, double[] y)

I'd propose
  public static double[] cat(double[] ... arrays)
Java 8 has an addAll method that is similar to the cat method:

http://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#addAll-int-java.util.Collection-

Perhaps use a similar naming convention...

The cat API is also does what Javascript's splice method does, so that might be 
a better name.
http://www.w3schools.com/jsref/jsref_splice.asp




/**
  * Returns an array consisting of the unique values in {@code data}.
  * The return array is sorted in descending order.
  *
  * @param data
  * @return descending list of values included in the input array
  */
public static double[] values(double[] data)

I'd suggest
  public static double[] uniq(double[] data)

Just a note - Java 8 Streams make the implementation fairly short:

|Integer[]array =newInteger[]{5,10,20,58,10};Stream.of(array).distinct().forEach(i 
->System.out.print(" "+i)); 5, 10, 20, 58 //Printed |


Cheers,
- Ole



/**
  * Adds random jitter to {@code data} using deviates sampled from
{@code dist}.
  * <p>
  * Note that jitter is applied in-place - i.e., the array
  * values are overwritten with the result of applying jitter.</p>
  *
  * @param data input/output data array
  * @param dist probability distribution to sample for jitter values
  * @throws NullPointerException if either of the parameters is null
  */
 public static void jitter(double[] data, RealDistribution dist)

IMO, this method should be part of the new API proposed in

  https://issues.apache.org/jira/browse/MATH-1158

Like so:

/** {@inheritDoc} */
public RealDistribution.Sampler createSampler(final RandomGenerator rng) {
    return new RealDistribution.Sampler() {
        public double next() { /* ... */ }
        public double[] next(int sampleSize) { /* ... */ }

        /**
         * @param data input/output data array.
         * @param dist probability distribution to sample for jitter values
         * @throws NullPointerException if data array is null.
         */
         public void jitter(double[] data) {
             final int len = data.length;
             final double[] jit = next(len);
             for (int i = 0; i < len; i++) {
                 data[i] += jit[i];
             }
         }
    };
}

Advantages:
* Simpler to use (half as many arguments). :-)
* Utility is defined where the base functionality is defined.
* Avoid adding to the overcrowded MathArrays utility class.
* Avoid dependency to another package (will help with modularization).

Drawbacks from usage POV
* None (?)

Drawbacks from implementation POV
* Cannot be done before we agree to go forward with the aforementioned issue.

In the meantime, I suggest to implement it as a "private" method.


Regards,
Gilles


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



Reply via email to