In C4/Overdues.pm: 574: # we're updating an existing fine. Only modify if amount changed 577: if ( $data->{'amount'} != $amount ) { for some numbers (e.g. 5.60) the comparison in line 577 fails ($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be a floating point number with no exact binary representation). E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3% of the amounts (on average). Depending on the fine charging settings, this may trigger unnecessary database writes (and a lot of them). If 'Fine Charging Interval' is 1 day, this has very little practical consequences (misc/cronjobs/fines.pl script is usually run once per day, and all fines need an increase anyway). But if the charging interval is longer, e.g. 7 days, in a given day amount changes only for the 1/7 of the fines (again, on average), and over 60% of the database writes would be redundant. To reproduce: 1) run misc/cronjobs/fines.pl 2) add warn "AMOUNT ".$data->{'amount'}." vs $amount"; below line 577 in C4/Overdues.pm 3) run misc/cronjobs/fines.pl again 4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6' or: dump accountlines table after step 1), run the script 2nd time, dump the table again and compare the contents - some of the records would have a different timestamp.
Created attachment 54532 [details] [review] Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need In C4/Overdues.pm: 574: # we're updating an existing fine. Only modify if amount changed 577: if ( $data->{'amount'} != $amount ) { for some numbers (e.g. 5.60) the comparison in line 577 fails ($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be a floating point number with no exact binary representation). E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3% of the amounts (on average). Depending on the fine charging settings, this may trigger unnecessary database writes (and a lot of them). If 'Fine Charging Interval' is 1 day, this has very little practical consequences (misc/cronjobs/fines.pl script is usually run once per day, and all fines need an increase anyway). But if the charging interval is longer, e.g. 7 days, in a given day amount changes only for the 1/7 of the fines (again, on average), and over 60% of the database writes would be redundant. To reproduce: 1) run misc/cronjobs/fines.pl 2) add warn "AMOUNT ".$data->{'amount'}." vs $amount"; below line 577 in C4/Overdues.pm 3) run misc/cronjobs/fines.pl again 4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6' or: dump accountlines table after step 1), run the script 2nd time, dump the table again and compare the contents - some of the records would have a different timestamp. To test: 1) apply patch 2) redo steps 1) - 4) above 3) no more warnings in step 4 4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster
(In reply to Jacek Ablewicz from comment #1) > 4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster On my test setup (19200 overdues, ~6500 redundant updates, FinesLog disabled) fines.pl run time was 90 seconds (before) vs 50 seconds (after). Not very striking difference, but this is on server with SSD drive, and innodb_flush_log_at_trx_commit = 2 in mysql config; YMMV.
(In reply to Jacek Ablewicz from comment #1) > Depending on the fine charging settings, this may trigger unnecessary > database writes (and a lot of them). If 'Fine Charging Interval' is > 1 day, this has very little practical consequences (misc/cronjobs/fines.pl > script is usually run once per day, and all fines need an increase > anyway). After a closer look - this is not entirely true. If the library has fine caps enabled (for individual items, or a total limit for all overdues), unnecessary database writes may happen there as well, even if 'Fine Charging Interval' is 1 day.
+ if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) { Why not force them both to sprintf and get rid of the hardcoded 6 ? Or would rounding with Math::Round::nearest be better than sprintf ? my $info = $schema->source('Accountline')->column_info('amount'); my $scale = $info->{size}->[1];
(In reply to Marcel de Rooy from comment #4) > + if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) { > Or would rounding with Math::Round::nearest be better than sprintf ? Quite possibly; sprintf('%.6f', ...) is not ideal, theoretically this comparison may still fail (but less often then currently) for some borderline cases, because mysql is supposedly using slightly different kind of rounding for internal decimal(x,y) arithmetic then sprintf (sprintf: half to even aka bankers rounding, mysql - half away from zero, commercial rounding). If someone is having numbers like 1.1212125 (more then 6 fractional digits, and the 7th digit is 5) in circulation & fine rules, calculated fine amount may get rounded differently. In such case though, fines.pl being not always as fast as humanly possible would be the least of his/her problems ;) > Why not force them both to sprintf and get rid of the hardcoded 6 ? I guess this can be done in other ways like abs($amount1 - $amount2) < 0.000001 but IMO that specific .6 precision is kind of important? > my $info = $schema->source('Accountline')->column_info('amount'); > my $scale = $info->{size}->[1]; This (and adding a new module for more predictable rounding etc.) would be a good start for solving all prices / fines rounding problems in Koha, once and for all.. I'm just not entirely happy with starting such monumental task right in this report, looks like a bit of an overkill to me.
(In reply to Jacek Ablewicz from comment #5) > This (and adding a new module for more predictable rounding etc.) would be a > good start for solving all prices / fines rounding problems in Koha, once > and for all.. I'm just not entirely happy with starting such monumental task > right in this report, looks like a bit of an overkill to me. Agreed. But replacing a hardcoded 6 by one or two lines might not be that monumental ?
(In reply to Marcel de Rooy from comment #6) > (In reply to Jacek Ablewicz from comment #5) > > This (and adding a new module for more predictable rounding etc.) would be a > > good start for solving all prices / fines rounding problems in Koha, once > > and for all.. I'm just not entirely happy with starting such monumental task > > right in this report, looks like a bit of an overkill to me. > > Agreed. But replacing a hardcoded 6 by one or two lines might not be that > monumental ? Ok, I'm happy with that; I don't have any strong opinion either way. My gut feeling though is that it would be better to have this in some kind of module, so it will be reusable. Otherwise, I'm worried that something like that would need to be included in each and every places where calculations in Koha are being tinkered with. But I'll better check first if those DBIx calls are not too costly. One never knows - this DBIx stuff is full of surprises, and not always the pleasant ones..
Comment on attachment 54532 [details] [review] Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need Review of attachment 54532 [details] [review]: ----------------------------------------------------------------- ::: C4/Overdues.pm @@ +574,4 @@ > # we're updating an existing fine. Only modify if amount changed > # Note that in the current implementation, you cannot pay against an accruing fine > # (i.e. , of accounttype 'FU'). Doing so will break accrual. > + if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) { While this does work, would not a compare_numbers($n1,$n2,$accuracy) function work better? Let's not rely on implicit type conversions for $data->{'amount'}.
(In reply to M. Tompsett from comment #8) > Comment on attachment 54532 [details] [review] [review] > Bug 17138 - UpdateFine() modyfies existing fine records even when there is > no need > > Review of attachment 54532 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: C4/Overdues.pm > @@ +574,4 @@ > > # we're updating an existing fine. Only modify if amount changed > > # Note that in the current implementation, you cannot pay against an accruing fine > > # (i.e. , of accounttype 'FU'). Doing so will break accrual. > > + if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) { > > While this does work, would not a compare_numbers($n1,$n2,$accuracy) > function work better? I'm not familiar with this function (compare_numbers), and google doesn't yield any (useful) results; from which module is it?
(In reply to Jacek Ablewicz from comment #9) > (In reply to M. Tompsett from comment #8) > > Comment on attachment 54532 [details] [review] [review] [review] > > Bug 17138 - UpdateFine() modyfies existing fine records even when there is > > no need > > > > Review of attachment 54532 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: C4/Overdues.pm > > @@ +574,4 @@ > > > # we're updating an existing fine. Only modify if amount changed > > > # Note that in the current implementation, you cannot pay against an accruing fine > > > # (i.e. , of accounttype 'FU'). Doing so will break accrual. > > > + if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) { > > > > While this does work, would not a compare_numbers($n1,$n2,$accuracy) > > function work better? > > I'm not familiar with this function (compare_numbers), and google doesn't > yield any (useful) results; from which module is it? I was implying writing it... like... wait for it...
Created attachment 58559 [details] [review] Bug 17138: Add and use comparison function and tests Added C4::Koha::compareNumbers($n1,$n2,$accuracy). Added corresponding tests to t/Koha.t Tweaked C4/Overdues to use the new function.
Created attachment 59755 [details] [review] [SIGNED-OFF] Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need In C4/Overdues.pm: 574: # we're updating an existing fine. Only modify if amount changed 577: if ( $data->{'amount'} != $amount ) { for some numbers (e.g. 5.60) the comparison in line 577 fails ($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be a floating point number with no exact binary representation). E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3% of the amounts (on average). Depending on the fine charging settings, this may trigger unnecessary database writes (and a lot of them). If 'Fine Charging Interval' is 1 day, this has very little practical consequences (misc/cronjobs/fines.pl script is usually run once per day, and all fines need an increase anyway). But if the charging interval is longer, e.g. 7 days, in a given day amount changes only for the 1/7 of the fines (again, on average), and over 60% of the database writes would be redundant. To reproduce: 1) run misc/cronjobs/fines.pl 2) add warn "AMOUNT ".$data->{'amount'}." vs $amount"; below line 577 in C4/Overdues.pm 3) run misc/cronjobs/fines.pl again 4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6' or: dump accountlines table after step 1), run the script 2nd time, dump the table again and compare the contents - some of the records would have a different timestamp. To test: 1) apply patch 2) redo steps 1) - 4) above 3) no more warnings in step 4 4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59756 [details] [review] [SIGNED-OFF] Bug 17138: Add and use comparison function and tests Added C4::Koha::compareNumbers($n1,$n2,$accuracy). Added corresponding tests to t/Koha.t Tweaked C4/Overdues to use the new function. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
(In reply to Josef Moravec from comment #13) > Created attachment 59756 [details] [review] [review] > [SIGNED-OFF] Bug 17138: Add and use comparison function and tests I think you should use Number::Format instead of creating a new subroutine. In any cases I am pretty sure that C4::Koha is the wrong place to put it in :)
I've implemented a quick fix for this using Koha::Number::Price::round to get us out of a hole.. But, I think it's a good idea to continue the work here to give us a comparison function in Koha::Number::Price.. I would suggest we stick to the semantics of perl's compare to allow for use in sorts in the future should we ever need that.
I'm now thinking of just making Koha::Number::Price a more formal object we can adopt and getting current functions that return 'numbers' to return Koha::Number::Price objects and then overload the various comparison and mathematic operators to 'do the right thing'... something a bit similar to how DateTime overloads comparison operators.
I have a working draft of an alternative solution to this available here: https://gitlab.com/mrenvoize/Koha/-/merge_requests/2 It would be great to have some feedback.
Never did get feedback I asked for [U+1F614]