Re: [lmi] Numerics

[Vadim Zeitlin]

On Sun, 8 May 2016 00:57:07 +0000 Greg Chicares <address@hidden> wrote:

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

 When hesitating about whether this ctor should exist or not, I thought
about a GUI control for entering dollars and cents separately (e.g. a
"masked text edit" with a fixed point before the last two positions). But I
admit I don't know if it's really a good reason to provide it.

 To be honest, my main motivation was probably the fact that I thought it
was "natural" to want to create a currency amount from dollars and cents.
If you think it isn't and if you don't think it's worth to provide it for
the use from the GUI code in the future, I'm going to remove it -- but
considering the time I already spent on writing/testing it, I'd appreciate
a confirmation of this.

GC> It could even be merged into operator>>() if that makes things simpler.

 Yes, I'll probably do just this then.

GC> Negative currency amounts are useful in practice.
...
GC> the stream inserter and extractor need to support negative currency amounts.

 OK, I got at least something right, as I hesitated about this too, but not
for a very long time, advantages of providing a well-defined subtraction
operation were quite clear.

GC> Can the "using" typedef and the "constexpr" things be made private?

 They could, but I think they should be public because it can be useful to
this class clients. The constants probably won't be that much if the ctor
from dollars+cents doesn't exist any more, but the underlying type still
seems to be useful.

GC> Oh, and, BTW, doesn't C++11 have static assertions that would let you
GC> express this comment:
GC>   static constexpr int subunits_per_unit = 100; // std::pow(10, 
subunits_digits)
GC> in code?

 Actually in real C++11 I could have just written the commented out
expression and it would be evaluated at compile-time and this does work
with g++ 4.9. Unfortunately it doesn't with MSVS 2015, so I did this
temporarily and I hope we can keep it just to make my life slightly easier,
especially because I hope it could be removed with a next version of MSVS
(and maybe even with the Update 2 which I don't have yet, but I could test
with it if it's important). But even if you really object to it, I'd rather
just remove "100" and keep MSVS workaround locally, but I don't think it's
worth using static_assert here.

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

 Again, for me these methods are useful for a GUI showing the amount in
dollars and cents.

GC>   amount_type total_subunits() const

 This one is admittedly mostly an implementation detail but it seems pretty
harmless to have and it makes writing tests much simpler. Could we please
keep it?
 
GC> Here:
GC>     double value() const
GC>         {
GC>         double result = subunits_;
GC>         result /= subunits_per_unit;
GC>         return result;
GC>         }
GC> of course
GC>   {return subunits_ / subunits_per_unit;}
GC> would perform integer division, which isn't wanted, and
GC>   {return (double)subunits_ / subunits_per_unit;}
GC> is ugly, while static_cast<double> is rather verbose. But is there a
GC> particular reason why you didn't write, say,
GC>     double result = subunits_;
GC>     return result / subunits_per_unit;
GC> (e.g., because it optimizes better), or am I looking for some profound
GC> subtlety there that doesn't actually exist?

 No, there are no subtleties here, I just wrote it like this because it
seemed more clear to me.

GC> At any rate, what about:
GC>   static constexpr double cents_to_dollars = 1.0 / subunits_per_unit;
GC>   ...
GC>   double value() const {return subunits_ * cents_to_dollars;}
GC> ? Then the compiler performs conversion to double implicitly, and it
GC> uses a multiplication instead of a division, which might still be
GC> faster even in the twenty-first century. (Or maybe it's the opposite
GC> because 100.0 has few significant binary mantissa bits, while 0.01
GC> has many; unit tests would inform us.)

 I am almost sure it's not going to make absolutely any difference and the
compiler will do whatever it considers to be best, but I'll benchmark it
just in case.

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

 No, this member has a default initializer, see

        https://github.com/vadz/lmi/blob/currency-class/currency.hpp#L180

FWIW I think it's a good idea to always provide default initializers for
all non-object fields in C++11, even if there is just a single ctor
initializing them anyhow because it makes it impossible to leave a field
uninitialized, e.g. by adding another ctor later.

GC> Here:
GC>     // Static constructor from the fractional amount of units rounding them 
to
GC>     // the nearest subunit. The argument may be positive or negative.
GC>     static currency from_value(double d)
GC>         {
GC>         if(std::trunc(d) >= static_cast<double>(max_units())) [throw 
exception]
GC> would -DBL_MAX elude that test?

 Oops. Of course it will, but, worse, so will anything less -LLONG_MAX/100.
There is a missing std::abs() call here, thanks for noticing it!

GC> Is there a reason to use std::trunc() to test overflow, but
GC> std::round() for the conversion

 Yes: trunc() is used for limit checking to ensure we don't overflow the
maximal amount_type value by multiplication (although I'm almost sure that
this would still be true even with round(), but using trunc() is supposed
to make this more obvious -- apparently not quite successfully), while
round() is used to give the expected result when creating currency from
non-integral-cents amounts.

GC> i.e., is there room for an error to slip through between the slightly
GC> different semantics of those two library functions (depending on the
GC> hardware rounding-direction bit)?

 Actually neither trunc() nor round() are supposed to depend on the
hardware rounding more in C++11. Whether it's the case in practice is
another question, of course...

GC> I'm not comfortable using the MinGW-W64 implementation of std::round()
GC> because it doesn't pass the 'round_test.cpp' unit tests that I wrote for the
GC> mingw.org implementation (which does pass).

 Oops, this is bad news and I was completely unaware of it. I hoped we
could finally abandon workarounds for old compilers with C++11, it's
disappointing to learn that we still can't do it.

GC> At least until the MinGW-W64 std::round() implementation is validated
GC> or corrected, I prefer to use lmi's 'round_to.hpp' facility.

 Which of the rounding functions defined there would you prefer to use
here?
 
GC> Is it actually a good idea for from_value(double d) to perform rounding?

 I thought about this and decided that this was the most reasonable choice
because otherwise we would need to arbitrarily fix the "maximal allow
distance from the nearest integer" and I don't see any good way to do it.

GC> With frequency in (usually,always] its argument should be the closest
GC> representable double <d> to the "true" value <D> that's really meant; thus:
GC>   D = 1.23 (the real number 123/100)
GC>   d = 1.229999999999996
GC> or at least a very near double, say within a relative error of 1E-14

 Well, this is the problem: where does this value come from exactly? 1 ULP
is not enough as more significant errors can easily accumulate and anything
that is neither 0 nor 1 (nor e nor pi, but those don't seem appropriate
here) is arbitrary.

GC> As a concrete example, accumulate $123.45 at an
GC> interest rate specified to eight decimals:
GC>   12345 cents * (1 + .0024662698) = 12375.446100681 = how many cents?

 For me the answer is "45" and I don't know what else can it be. Of course,
I could be just very naïve.

GC> In actual practice, we specify whether that's to be rounded up, down, or
GC> to nearest, and to how many decimals, so these outcomes seem desirable:
GC>   factor u = 1 + .0024662698;
GC>   currency x = 123.45; // Okay.
GC>   x *= u;    // Error: rounding details not specified.
GC>   x = x * u; // Error: rounding details not specified.
GC>   x = round_to_currency_some_specific_way(x * u) // Okay.

 Notice that with the current class you can't do "x*u" at all (only
multiplication by an integer is supported), so my from_value() is basically
just your round_to_currency_some_specific_way() which always rounds to the
nearest cent, away from zero in halfway cases.

 If we need other rounding modes, I think they should be provided by this
class itself, e.g. from_value() could take rounding_style, although this is
not without its own questions: should it have a default value? What should
happen in r_not_at_all case?

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

 Frankly, if we don't care about any other currencies, we could just as
well use dollars and cents, of course. I don't like the other suggestions
at all, the only other currency name I'd consider would be Zorkmid...

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

 This can, of course, be committed as is, it's completely independent of
all the rest, but it seems there are quite a few changes to be made (and at
least one obvious bug with the negative amounts in from_value()), so
perhaps it would be better to make them first? Of course, I could do them
later too, so it doesn't really matter as long as we both agree what are we
doing. Please let me know what would you prefer and also the answers to the
questions above, i.e., in order of priority:

0. Whether you're sure we don't need ctor from dollars+cents.
1. What would you prefer to call units/subunits.
2. What API for rounding would you like to see.

 Thanks in advance!
VZ