Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> These results are based on the attached, updated patch which includes
> a few minor improvements.
I started to look at this patch, and was immediately bemused by the
comment in estimate_ln_weight:
/* * 0.9 <= var <= 1.1 * * Its logarithm has a negative weight (possibly very large).
Estimate * it using ln(var) = ln(1+x) = x + O(x^2) ~= x. */
This comment's nonsense of course: ln(0.9) is about -0.105, and ln(1.1) is
about 0.095, which is not even negative much less "very large". We could
just replace that with "For values reasonably close to 1, we can estimate
the result using ln(var) = ln(1+x) ~= x." I am wondering what's the point
though: why not flush this entire branch in favor of always using the
generic case? I rather doubt that on any modern machine two uses of
cmp_var plus init_var/sub_var/free_var are going to be cheaper than the
log() call you save. You would need a different way to special-case 1.0,
but that's not expensive.
Larger questions are
(1) why the Abs() in the specification of estimate_ln_weight --- that
doesn't comport with the text about "Estimate the weight of the most
significant digit". (The branch I'm proposing you remove fails to
honor that anyway.)
(2) what should the behavior be for input == 1, and why? The code
is returning zero, but that seems inconsistent or at least
underdocumented.
(3) if it's throwing an error for zero input, why not for negative
input? I'm not sure that either behavior is within the purview of
this function, anyway. Seems like returning zero might be fine.
regards, tom lane