> On May 19, 2018, at 2:28 AM, Geert Janssens <geert.gnuc...@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
_______________________________________________
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
