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" <gil...@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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
