[GNC-dev] gnc_numeric division with how=GNC_HOW_DENOM_LCD

[John Ralls]

> On May 19, 2018, at 2:28 AM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:
> 
> Christopher Lam is busy writing unit tests for our business reports which is 
> great.
> 
> However he has run into  a peculiar issue in that he can't create a test 
> invoice entries that properly handle tax or percentages.
> 
> I have been tracing in the debugger and I ran into a peculiar bug for which I 
> need some feedback.
> 
> The core of the issue is in this line:
> https://github.com/Gnucash/gnucash/blob/maint/libgnucash/engine/
> gncEntry.c#L1139
> 
> This division is to convert a number as entered by the user into a percentage 
> value.
> The user enters "10", which the code should interpret as 10%, or 0.01
> 
> This works fine with numbers entered by real users in the user interface, 
> however it fails when generating these same numbers via scheme code.
> 
> And this is because both get a different denominator: all user entered numbers 
> (in this context) will be stored with a denom of 100000. However when you make 
> a scheme rational 1000000/100000 (so equivalent to 10), it gets reduced to 
> 10/1 by the time it's used in the line above. Clearly guile automatically 
> reduces rationals.
> 
> The net effect is that for user entered numbers the line above will divide 
> 1000000/100000 by 100/1 while for scheme generated numbers the division 
> becomes 10/1 by 100/1.
> 
> Combining this with how=GNC_HOW_DENOM_LCD makes that the denominator will be 
> fixed to 1 causing 0.1/1 to be rounded to 0.
> 
> That's the symptom. But I do have some difficulty determining the actual 
> cause: is gnc_numeric_div doing the wrong thing with GNC_HOW_DENOM_LCD ? That 
> would make it a bug in gnc_numeric_div. In a division I would expect the 
> nominator of the divisor to be taken into account to calculate the LCD but 
> perhaps my (accounting related) math is wrong.
> Or perhaps GNC_HOW_DENOM_LCD just doesn't make sense for a division and 
> another rounding method should be used here? That would make it a bug in the 
> gncEntryComputeValueInt function.
> Or both are right and I should just ensure to have a proper common denominator 
> somehow. I could maybe set percent in the calculation above to 10000000/100000 
> instead of to 100/1.
> Or I could avoid gnc_numeric_div altogether by multiplying with 1/100 instead.
> 
> However I wanted some feedback before applying one of the workarounds to be 
> sure I'm not masking a a more important bug that way.
> 

For reference, the line in question is 

tpercent = gnc_numeric_div (tpercent, percent, GNC_DENOM_AUTO,
                                            GNC_HOW_DENOM_LCD);

Note in particular the warning at https://github.com/Gnucash/gnucash/blob/maint/libgnucash/engine/gnc-numeric.h#L80 <https://github.com/Gnucash/gnucash/blob/maint/libgnucash/engine/gnc-numeric.h#L80>
that the fractional part of a number may be discarded if you don’t specify a rounding policy. This call has a denominator setting but no rounding policy.

I think GNC_HOW_DENOM_LCD doesn’t make sense for most divisions, it’s very likely that one wants more precision than the smaller of the operands' denominators. I think for this case you want GNC_HOW_DENOM_EXACT | GNC_HOW_ROUND_NEVER or GNC_HOW_DENOM_REDUCE | GNC_HOW_ROUND_NEVER: That preserves all of the precision that a user enters, though it does present the (small) possibility of an overflow error. GNC_HOW_DENOM_REDUCE | GNC_HOW_ROUND_RND will trade the small possibility of an overflow for the small possibility of a rounding error in the actual tax calculation.

Multiplying 10/1  by 1/100 won’t change anything: The LCD is still 1.

Regards,
John Ralls