On Mon, 08 May 2017 14:16:12 +0200, Gilles <gil...@harfang.homelinux.org> wrote:
> > Overall the addition looks good and well commented. The javadoc is a
> > bit
> > sparse and could do with some more hyperlinks.
> Strange; I thought that it was doing rather well on this point.
> Can you be more specific? [Please open a JIRA report.]
Just thought it would be good with a quick one-liner about what the
gamma function is rather than a link to Wolfram Alpha. I added something
generic like:
* The Gamma function can be seen to extend the factorial function to cover
* real and complex numbers, but with its argument shifted by
* {@code * -1}. This implementation supports real numbers.
(BTW - why does it not support complex numbers?)
> The original (CM) design contained in single big class and static
> methods for all the functions.
> I thought it would be clearer to split it into several classes (each
> with its own unit test class).
Yes, this is a very good design choice, I agree with this. Part of the
javadoc improvements I tried to add is to also then relate the new
classes to eachother, so I added say a @see Gamma back from Digamma.
> One goal was to switch away from static methods, to allow an easy path
> to using the Java 8 functional interfaces.
> [Help is most welcome to check that it will indeed be the case.]
Hm, good insentive - but I don't think this helps much.. The only way I
could use it now is to make a new instance, which looks awkward:
import org.apache.commons.numbers.gamma.Gamma;
@Test
public void streamGamma() throws Exception {
List<Double> numbers =
Arrays.asList(-2.1,-2.0,-1.99,-1.95,-1.9,-1.8,-1.7,-1.6,-1.5,-1.4,-1.3,-1.2,-1.1,-1.0,-0.9);
Stream<Double> d = numbers.parallelStream().map(new Gamma()::value);
d.forEachOrdered(System.out::println);
}
Passing just "new Gamma()" won't work as it does not implement any
@FunctionalInterface.
If I make the method static and rename it to "gamma", then I can do the much
simpler:
Stream<Double> d = numbers.parallelStream().map(Gamma::gamma);
.. as well as import it as a static reference (however Java 8 won't allow
passing just "gamma").
I can only see this "value()" style make sense if there is also a common
interface, perhaps DoubleFunction<Double>?
https://docs.oracle.com/javase/8/docs/api/java/util/function/DoubleFunction.html
.. but this uses apply(x).
> I've fixed "Digamma" and "Trigamma" that still inadvertently contained
> static methods.
> Thanks for spotting it.
Hm, but you fixed it the wrong way? :) Now they are all called value() and are
non-static.. why do we need to make instances of these classes which only state
is to other static helper classes?
> The "instance" field is for use "helper" in other classes in the
> package,
> to avoid a few unnecessary instantiations.
> But I tend to agree that it is perhaps not worth it.
If the class has a public constructor there will be many instansiations of the
class.
Now it is quite hard to check the class is actually immutable, as we have to
check these fields.
I know there many good arguments against implicit static initializers, as it
can be quite tricky to determine the initialization order. Referring to such
instances from static fields can cause such issues. (I don't think that's the
case here though).
> Cf. above.
> I'd be willing to make this package target Java 8. [Any objections?]
+100! (All of Commons Numbers, presumably)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
