You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gilles <gi...@harfang.homelinux.org> on 2017/05/08 00:30:32 UTC
[Numbers] NUMBERS-33
Hi.
Please review branch "task_NUMBERS-33__Gamma":
https://issues.apache.org/jira/browse/NUMBERS-33
Thanks,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Numbers] NUMBERS-33
Posted by Stian Soiland-Reyes <st...@apache.org>.
On Tue, 9 May 2017 13:29:21 +0100, Stian Soiland-Reyes <st...@apache.org> wrote:
> 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
DoubleUnaryOperator would be better - double applyDouble(double) - then
Gamma can be used like this with a native DoubleStream:
double[] numbers = { -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 };
Arrays.stream(numbers).map(new Gamma())
.mapToObj(Double::toString)
.forEachOrdered(System.out::println);
Or even -- if you permit:
DoubleStream d = new Random().doubles().map(new Gamma());
https://docs.oracle.com/javase/8/docs/api/java/util/function/DoubleUnaryOperator.html
Not sure I like the function name "applyDouble(x)" though.. We could have a
static gamma() method (etc) as well.
===
One thought (which don't work too well with Streams without helper functions or
Gamma::new)):
If we are making instances of Gamma, then you could make it extend Number and
take the "x" argument in the constructor:
Number g = new Gamma(0.4);
System.out.println(g.floatValue());
Evaluation of the gamma function can be delayed until the first call on any of
the methods.
This is the style recommended by Elegant Objects:
http://www.yegor256.com/2014/05/05/oop-alternative-to-utility-classes.html
.. and would allow us to extend it to complex numbers in the future.
--
Stian Soiland-Reyes
The University of Manchester
http://www.esciencelab.org.uk/
http://orcid.org/0000-0001-9842-9718
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Numbers] NUMBERS-33
Posted by Raymond DeCampo <ra...@decampo.org>.
On Wed, May 10, 2017 at 10:49 AM, Gilles <gi...@harfang.homelinux.org>
wrote:
> Hi.
>
>
> On Tue, 9 May 2017 12:59:40 -0400, Raymond DeCampo wrote:
>
>> 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);
>>>
>>
>> It seems to me like the above, suggested by Stian earlier if I am not
>> mistaken, is the best option of the ones you have discussed, once you
>> agree
>> on a name for the static method.
>>
>> I don't think there is a need to explicitly implement DoubleUnaryOperator
>> as a method reference can always be implicitly converter by the compiler.
>>
>
> Is commit ef4ab32365ea7f5a0debb9f48734839b5ed00a86 OK (as far as usage
> with Java 8 is concerned)?
>
> If we decide to have "Numbers" target Java 8, we'll define an additional
> (non-static) "apply(double)" method that will call the static one.
>
> Regards,
> Gilles
Looks good to me.
Re: [Numbers] NUMBERS-33
Posted by Gilles <gi...@harfang.homelinux.org>.
Hi.
On Tue, 9 May 2017 12:59:40 -0400, Raymond DeCampo wrote:
>> 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);
>
> It seems to me like the above, suggested by Stian earlier if I am not
> mistaken, is the best option of the ones you have discussed, once you
> agree
> on a name for the static method.
>
> I don't think there is a need to explicitly implement
> DoubleUnaryOperator
> as a method reference can always be implicitly converter by the
> compiler.
Is commit ef4ab32365ea7f5a0debb9f48734839b5ed00a86 OK (as far as usage
with Java 8 is concerned)?
If we decide to have "Numbers" target Java 8, we'll define an
additional
(non-static) "apply(double)" method that will call the static one.
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Numbers] NUMBERS-33
Posted by Raymond DeCampo <ra...@decampo.org>.
> 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);
It seems to me like the above, suggested by Stian earlier if I am not
mistaken, is the best option of the ones you have discussed, once you agree
on a name for the static method.
I don't think there is a need to explicitly implement DoubleUnaryOperator
as a method reference can always be implicitly converter by the compiler.
Re: [Numbers] NUMBERS-33
Posted by Stian Soiland-Reyes <st...@apache.org>.
On Tue, 09 May 2017 14:52:09 +0200, Gilles <gi...@harfang.homelinux.org> wrote:
> [I specifically avoided "apply" so that it is free for when the
> component
> targets Java 8.]
>
> Can the "apply" method be static?
No, interface implementations can't be static (and it is not possible to
make an interface of static methods).
> Won't a private constructor (which was my original intention) preclude
> some of the functional usages?
> If not, I'll make the change
Yes, that would make it harder to do say a collection of functions that
you combine in a stream. But they need a common interface, otherwise
they will just happen to have the same name and extend
java.lang.Object.. (If we upgrade to Java8 we won't need to make such
an interface ourselves)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Numbers] NUMBERS-33
Posted by Gilles <gi...@harfang.homelinux.org>.
Hi.
On Tue, 9 May 2017 13:29:21 +0100, Stian Soiland-Reyes wrote:
> On Mon, 08 May 2017 14:16:12 +0200, Gilles
> <gi...@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
Re: [Numbers] NUMBERS-33
Posted by Stian Soiland-Reyes <st...@apache.org>.
On Mon, 08 May 2017 14:16:12 +0200, Gilles <gi...@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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Numbers] NUMBERS-33
Posted by Gilles <gi...@harfang.homelinux.org>.
Hello Stian.
On Mon, 8 May 2017 10:01:48 +0100, Stian Soiland-Reyes wrote:
> Thanks!
Thanks for taking the time to review. :-)
>
> Not being a mathematician I am afraid I have not been able to review
> the
> correctness of the equation use.
I didn't either; I assumed to be correctness as per years of presence
in CM (up to 6 releases old).
>
> 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.]
>
> I have some code design questions you may be able to clarify:
>
> * Why have static methods all called value()?
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).
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.]
> Static methods are good as
> they can be imported independently (and without wasting object
> state), yet
> by calling all the public methods .value() only one can be imported,
> and
> with a meaningless name; we are forced instead to always call them
> via the
> class Gamma.value()
I've fixed "Digamma" and "Trigamma" that still inadvertently contained
static methods.
Thanks for spotting it.
>
> * Why do some of the package private classes (e.g. InvGamma1pm1) have
> a
> needless static singleton instance and a single method (and no
> state), when
> here as well a static method would do?
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.
> The only reason I can see for this
> is to implement interfaces or subclasses, yet none of those seem to
> be
> present, and these could rather follow the same pattern as the public
> API
> with private constructors.
Cf. above.
I'd be willing to make this package target Java 8. [Any objections?]
>
> Won't this loop loose precision on digamma? (Could be called up to 49
> times)
>
> while (x < C_LIMIT) {
> + digamma -= 1 / x;
> + x += 1;
> + }
No idea.
Please add your concern here:
https://issues.apache.org/jira/browse/NUMBERS-35
>
> In this loop I was confused as the formula does not at first seem to
> match
> the code:
> final double inv = 1 / (x * x);
> + // 1 1 1 1
> + // log(x) - --- - ------ + ------- - -------
> + // 2 x 12 x^2 120 x^4 252 x^6
> + digamma += Math.log(x) - 0.5 / x - inv * (F_1_12 + inv * (F_1_120 -
> F_1_252 * inv));
> +
> + return digamma;
Nice catch, indeed.
[Ah, the virtue of refactoring, and team work!]
Now fixed (but untested, cf. JIRA issue above).
>
> Here and other places:
> /** Helper. */
> + private static final InvGamma1pm1 INV_GAMMA_1P_M1 =
> InvGamma1pm1.instance;
>
> Unless there were state to initialize in certain order I don't see
> the
> benefit of this field wastage over just calling InvGamma1pm1.value()
> directly.
Cf. above (functional interface goal).
>
> Based on the <em>NSWC Library of Mathematics Subroutines</em> double
> + * precision implementation, {@code DGAMMA}.
>
> What is the license/url of this nswc code? What is the nature of
> "based on"
> here?
That same code made it through releases since CM 3.2.
IMO, the comment is proof enough that licensing issue had been tackled
at the time.
In the CM reporsitory (class "o.a.c.m.special.Gamma"), there was a link
to the original source code:
http://www.ualberta.ca/CNS/RESEARCH/Software/NumericalNSWC/site.html
but that link is now dead. :-(
A quick search:
http://www.swmath.org/software/9119
[Help welcome to check that this is the same reference code.]
Thanks,
Gilles
>
>
> On 8 May 2017 1:30 am, "Gilles" <gi...@harfang.homelinux.org> wrote:
>
> Hi.
>
> Please review branch "task_NUMBERS-33__Gamma":
> https://issues.apache.org/jira/browse/NUMBERS-33
>
> Thanks,
> Gilles
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Numbers] NUMBERS-33
Posted by Stian Soiland-Reyes <st...@apache.org>.
Thanks!
Not being a mathematician I am afraid I have not been able to review the
correctness of the equation use.
Overall the addition looks good and well commented. The javadoc is a bit
sparse and could do with some more hyperlinks.
I have some code design questions you may be able to clarify:
* Why have static methods all called value()? Static methods are good as
they can be imported independently (and without wasting object state), yet
by calling all the public methods .value() only one can be imported, and
with a meaningless name; we are forced instead to always call them via the
class Gamma.value()
* Why do some of the package private classes (e.g. InvGamma1pm1) have a
needless static singleton instance and a single method (and no state), when
here as well a static method would do? The only reason I can see for this
is to implement interfaces or subclasses, yet none of those seem to be
present, and these could rather follow the same pattern as the public API
with private constructors.
Won't this loop loose precision on digamma? (Could be called up to 49 times)
while (x < C_LIMIT) {
+ digamma -= 1 / x;
+ x += 1;
+ }
In this loop I was confused as the formula does not at first seem to match
the code:
final double inv = 1 / (x * x);
+ // 1 1 1 1
+ // log(x) - --- - ------ + ------- - -------
+ // 2 x 12 x^2 120 x^4 252 x^6
+ digamma += Math.log(x) - 0.5 / x - inv * (F_1_12 + inv * (F_1_120 -
F_1_252 * inv));
+
+ return digamma;
Here and other places:
/** Helper. */
+ private static final InvGamma1pm1 INV_GAMMA_1P_M1 = InvGamma1pm1.instance;
Unless there were state to initialize in certain order I don't see the
benefit of this field wastage over just calling InvGamma1pm1.value()
directly.
Based on the <em>NSWC Library of Mathematics Subroutines</em> double
+ * precision implementation, {@code DGAMMA}.
What is the license/url of this nswc code? What is the nature of "based on"
here?
On 8 May 2017 1:30 am, "Gilles" <gi...@harfang.homelinux.org> wrote:
Hi.
Please review branch "task_NUMBERS-33__Gamma":
https://issues.apache.org/jira/browse/NUMBERS-33
Thanks,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org