commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [Numbers] NUMBERS-33
Date Tue, 09 May 2017 12:52:09 GMT
Hi.

On Tue, 9 May 2017 13:29:21 +0100, Stian Soiland-Reyes wrote:
> On Mon, 08 May 2017 14:16:12 +0200, Gilles
> <gilles@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.

It can't hurt. ;-)

> (BTW - why does it not support complex numbers?)

New feature?

>> 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.

Thanks.

>
>> 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);

I don't like the "gamma" duplication.
I'll make the method "value" static to allow the above usage.

> .. 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 specifically avoided "apply" so that it is free for when the 
component
targets Java 8.]

Can the "apply" method be static?

>> 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?

Won't a private constructor (which was my original intention) preclude
some of the functional usages?
If not, I'll make the change

>> 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)

Not necessarily.
[Personally I don't mind, but Sebb might...]

Gilles


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


Mime
View raw message