Re: [lmi] Currency class

[Greg Chicares]

On 2020-09-17 11:40, Vadim Zeitlin wrote:
> On Thu, 17 Sep 2020 00:41:52 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> GC> Surely this can
> GC> be improved by rewriting the expression--perhaps
> GC> 
> GC>     currency max_loan =
> GC>           round_somehow.c(AVGenAcct + AVSepAcct) * MaxLoanAVMult)
> 
>  Another parenthetical aside: I believe there is a missing opening
> parenthesis here (and not an extra closing one), i.e. round_somehow()
> should apply to the whole expression.

Indeed. That's why I run a system test before each push (though not
before pressing "Send" in email).

>  Is there any kind of rule that could be associated with MaxLoanAVMult to
> indicate how should the result of multiplying by it be rounded? If there
> were, we could probably do something smart about doing it automatically.
> But if the results of multiplying by the same factor needs to be rounded
> differently depending on where it occurs, then I indeed don't see any other
> choice than reviewing all such expressions individually.

There is no general rule; but in this particular case, about fifty
lines later we get to:
    MaxLoan = round_loan().c(max_loan);
which is the answer to a question, but not the question you asked.
The answer to your question is "No", this subexpression:
          (AVGenAcct + AVSepAcct).d() * MaxLoanAVMult
should not be rounded. The actuaries who write specifications for
such calculations tend to prescribe complex formulas with rounding
applied only at the last step, and not to any intermediate value.

Although it's a major undertaking, lmi's life-insurance calculations
would benefit from a review of each function and maybe even each
expression. In this function, AccountValue::SetMaxLoan(), we see
that 'reg_loan_factor' and 'prf_loan_factor' are recalculated,
distressingly, by calling std::pow(), which is
 - always costly, and
 - probably less than ideally accurate (should be expm1() and log1p()).
And yet that costly recalculation isn't reachable because it's
preceded by
        alarum() << "Off-anniversary loans untested." << LMI_FLUSH;
and no user has ever reported that "assertion" firing, so it's
probably unreachable anyway. And then there's the cryptic

    // I do not think we want a MaxLoan < current level of indebtedness.
    MaxLoan = std::max((AVRegLn + AVPrfLn), MaxLoan);
    // TODO ?? Yet I do not think we want to ratify something that looks broken!

where the comments were written by two different people, a decade
or two ago; in effect, it says that there's a postcondition that
should be checked, but if it's violated, then we blithely alter
the postcondition. It's easy to fix that, by asserting the
postcondition, but maybe it was overridden for a (forgotten) good
reason that the comments fail to record...in which case the obvious
"fix" would introduce breakage.

Anyway, there's certainly a smaller, cleaner system trying to be
born. It'll just take a lot of labor.

> GC> But here's my dilemma. In the latest README.branch file, I use
> GC> objective data to make a case that using a currency class must
> GC> entail a fearsome performance penalty. Yet OTOH that simply
> GC> cannot be.
> 
>  This is indeed surprising, but I've vowed to never say "it cannot be"
> about any benchmarking results after seeing too many "impossible"
> measurements. If it does happen, all I can -- perfectly unhelpfully -- say
> is that it clearly can be. But there is still an interesting, and
> potentially helpful, question to answer, which is "how does it come to
> pass?". So, if this is the more or less final version of your code, I'd
> like to profile the code and see if I can discover where does this slowdown
> come from. Should I/we do this? If so, could you please explain how exactly
> do you obtain these measurements?

These measurements are just
  /opt/lmi/bin[0]$wine ./lmi_cli_shared.exe --accept --data_path=/opt/lmi/data 
--selftest

This afternoon I've pushed a series of commits that carve the
costly overhead into layers, in a way that I hope the commit
messages make clear. I've managed to improve the performance
considerably by discarding some layers that we'll ultimately
want to restore in some fashion. What I don't understand is
why these now-separated layers of overhead cost so much.
I would have thought that a function like
  inline double identity(double d) {return d;}
would have been optimized away, costing nothing. And I guess
I'm a little surprised that multiplying by 100.0 * 0.01
repeatedly costs so much.