Re: [lmi] Numerics

[Greg Chicares]

On 2016-05-07 18:16, Vadim Zeitlin wrote:
> On Mon, 2 May 2016 15:16:29 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> > so apart a unit test class
> GC> > for this class itself (which I'll write, but it will be pretty 
> trivial), I
> GC> > don't see what could I test.
> GC> 
> GC> ...if you'll write the unit test. I'm hoping that a unit test will prove 
> the
> GC> lack of any runtime penalty. Other lmi unit tests use a high-precision 
> timer
> GC> that might be useful here.
> 
>  I haven't yet done the performance measurement part as it turned out that
> writing this class was not as trivial as I thought, notably the constructor
> from dollars and cents (politically correctly called "units" and "subunits"
> in my code) was problematic as I wasn't sure if it should be allowed to
> create negative objects or even if it should be provided at all. Finally
> I've chosen to only allow positive amounts in this ctor to avoid confusion
> but I'm not sure at all if you agree with this decision, so I'd like you to
> have a look at it here:
> 
>       https://github.com/vadz/lmi/blob/currency-class/currency.hpp

I think that ctor should be private because it is only an implementation
detail of operator>>(). I can't imagine calling it elsewhere. In practice
we always have a scalar amount-of-money, never a pair of {dollars, cents}.
(That pair may arise when we buy something for cash and want to count out
the right number of bills and coins, but that's the only use case I know
for mapping currency amounts to such a pair.) It could even be merged into
operator>>() if that makes things simpler.

Negative currency amounts are useful in practice. Here's an example: a
contract has a $5 fee of some sort, built into the product files, and in
some particular case it is to be reduced to $3.50; instead of laboriously
editing the product files, we can enter a $-1.50 "extra fee" in the case's
input file, which is written as a currency amount with operator>>(). Thus,
the stream inserter and extractor need to support negative currency amounts.

Can the "using" typedef and the "constexpr" things be made private?
Oh, and, BTW, doesn't C++11 have static assertions that would let you
express this comment:
  static constexpr int subunits_per_unit = 100; // std::pow(10, subunits_digits)
in code?

I think these functions should be private:
  amount_type units() const
  int subunits() const
  amount_type total_subunits() const

Here:
    double value() const
        {
        double result = subunits_;
        result /= subunits_per_unit;
        return result;
        }
of course
  {return subunits_ / subunits_per_unit;}
would perform integer division, which isn't wanted, and
  {return (double)subunits_ / subunits_per_unit;}
is ugly, while static_cast<double> is rather verbose. But is there a
particular reason why you didn't write, say,
    double result = subunits_;
    return result / subunits_per_unit;
(e.g., because it optimizes better), or am I looking for some profound
subtlety there that doesn't actually exist? At any rate, what about:
  static constexpr double cents_to_dollars = 1.0 / subunits_per_unit;
  ...
  double value() const {return subunits_ * cents_to_dollars;}
? Then the compiler performs conversion to double implicitly, and it
uses a multiplication instead of a division, which might still be
faster even in the twenty-first century. (Or maybe it's the opposite
because 100.0 has few significant binary mantissa bits, while 0.01
has many; unit tests would inform us.)

In this case [reformatted as a single line]
  currency() {}
is it not necessary to initialize 'subunits_' explicitly to zero?

Here:
    // Static constructor from the fractional amount of units rounding them to
    // the nearest subunit. The argument may be positive or negative.
    static currency from_value(double d)
        {
        if(std::trunc(d) >= static_cast<double>(max_units())) [throw exception]
would -DBL_MAX elude that test? Is there a reason to use std::trunc() to
test overflow, but std::round() for the conversion--i.e., is there room for
an error to slip through between the slightly different semantics of those
two library functions (depending on the hardware rounding-direction bit)?

I'm not comfortable using the MinGW-W64 implementation of std::round()
because it doesn't pass the 'round_test.cpp' unit tests that I wrote for the
mingw.org implementation (which does pass). At least until the MinGW-W64
std::round() implementation is validated or corrected, I prefer to use lmi's
'round_to.hpp' facility.

Is it actually a good idea for from_value(double d) to perform rounding?
With frequency in (usually,always] its argument should be the closest
representable double <d> to the "true" value <D> that's really meant; thus:
  D = 1.23 (the real number 123/100)
  d = 1.229999999999996
or at least a very near double, say within a relative error of 1E-14 (with
special provision for arguments in the neighborhood of zero). If we had a
supplementary safe_from_value() function that asserts such a
near-quantization in the argument, then we might find that we always (or
usually) prefer the "safe" version, and it would be interesting to find a
practical rationale for preferring the "unsafe" behavior. Only in praxis
will we find the answer; if it turns out that the "safe" behavior is always
wanted, then the "unsafe" function might either become an implementation
detail or be removed. As a concrete example, accumulate $123.45 at an
interest rate specified to eight decimals:
  12345 cents * (1 + .0024662698) = 12375.446100681 = how many cents?
In actual practice, we specify whether that's to be rounded up, down, or
to nearest, and to how many decimals, so these outcomes seem desirable:
  factor u = 1 + .0024662698;
  currency x = 123.45; // Okay.
  x *= u;    // Error: rounding details not specified.
  x = x * u; // Error: rounding details not specified.
  x = round_to_currency_some_specific_way(x * u) // Okay.
A run-time check for a "minuscule" relative error does impose a cost, but
in practice addition and subtraction (without that cost) are common,
multiplication by a non-quantized factor like 1.0024662698 is rare, and
stream extraction and insertion are already costly and infrequently done.

Would it be politically-correct enough to use some other pair of terms whose
members are more immediately suggestive? Euros and cents, if EU-centrism is
acceptable; rubles and kopeks; or even "bills" and "coins" (unambiguous in
the US, where dollar coins are curiosities that aren't seen in circulation).
It would be much easier for me to follow if the terms were more evocative.
How about "units" and "hundredths", which isn't really US-centric because
even the UK now uses decimally-fractional currency; or "units" and "cents"?
Or, because lmi is inherently US-centric, "dollars" and "cents"?

> The full commit, including changes to the makefiles and the unit test is at
> 
>       
> https://github.com/vadz/lmi/commit/4b3acfb7544541d40f302dc824f1b423088f7fe4
> 
> and I've tested that the test passes both when cross-compiling and when
> using the native build system.
> 
>  Clearly, more things could be added to the class itself and the test (in
> addition to the performance tests mentioned above, I'd also like to add
> more extensive checks for other methods using random values), but I think
> this initial version could already profit from your review, so I'd be
> grateful if you could please let me know what do you think of it.

Can I commit it as is, without bruising your older patches? If that would
require rebasing older patches, then I'd rather just work through all of
them in order, once we're finished revising group quotes.