SA Bugzilla – Bug 2607
Fixing "X-Spam-Status: No, hits=5.0 required=5.0"
Last modified: 2004-09-07 19:46:30 UTC
I (and many others who don't understand rounding) would greatly appreciate if the hits variable could be rounded down to the closest 0.1 (rather than just rounding) so as to transform the subject phrase into the much clearer Fixing "X-Spam-Status: No, hits=4.9 required=5.0". I'm suggesting that new_hits = int(old_hits * 10) / 10, where int is defined at <http://www.cotse.com/perl/int.html> Int is not a true rounddown function, in that if old_hits is -4.54, new_hits will round up to -4.5 rather than down to -4.6, but that's a lot less counter-intuitive than what's there today. And yes, I know that the cited Perl documentation recommends sprintf instead of int, but in this case I think it will be much clearer to round down rather than just round. I could be completely wrong, but I believe the fix would be in PerMsgStatus.pm to round the hits variable down to the same level of precision by which it will be shown to users (rather than rounding to 3 decimal places here and 1 decimal place in the X-Spam-Status header): # Round the hits to 3 decimal places to avoid rounding issues # We assume required_hits to be properly rounded already. # add 0 to force it back to numeric representation instead of string. $self->{hits} = (sprintf "%0.3f", $self->{hits}) + 0; I believe this should be: $self->{hits} = int($self->{hits} * 10) / 10 but then I don't really know Perl so I wouldn't take my word for it. Another option would be: $self->{hits} = (sprintf "%0.1f", $self->{hits}) + 0; The only difference is: perl -e 'print sprintf ("%0.1f\n", 4.950)' 5.0 perl -e 'print (int(4.950*10)/10)' 4.9 I would argue the rounddown functionality of the latter is closer to the intent of a required hits function than rounding. Note that one other way of attacking the underlying issue (which is that the numbers used by the program logic are not the ones being presented to the user) would be to make the use of sprintf consistent throughout the module so that it used, say "%2.1f" in all cases, and not just for display.
Created attachment 1509 [details] Proposed patch file Here's a patch that implements my proposed change to fix the confusing rounding info.
This is a FAQ. I think the general consensus is that the rounding is confusing, but it's better than rounding down. People will start asking why is SpamAssassin rounding down instead of rounding properly, etc. :-) http://wiki.spamassassin.org/w/RoundingIssues
Seems like everybody agrees in the end :) Dan, do we have a CLA from you? (Bugzilla doesn't think so.)
I just faxed in my CLA.
I think Dan's right, this can go into 2.6x. Can somebody please confirm his CLA has arrived?
btw: if we decide this is a good idea (I'm not sure, but can't think of any concrete reasons why it's bad), we need to apply it to 3.0.0 as well. just so we don't forget. :)
>I think Dan's right, this can go into 2.6x. Can somebody please confirm his CLA has arrived? Well, the turnaround on checking CLAs is not quite so simple. We have to ask Jim Jagielski and wait a while for him to check the incoming fax queue ;) I suggest that if (a) the patch is going into 3.0.0 -- which has more lax requirements for CLAs due to the new license text -- and (b) the reporter has gone on record on the Bugzilla noting that they've sent the CLA, that's good enough. In this case anyway, the patch is very small. under ASL2 rules it doesn't strictly require a CLA for changes of this size.
oh, +1 to Dan's fix too
Committed to HEAD but decided against backporting.
*** Bug 3229 has been marked as a duplicate of this bug. ***
*** Bug 3757 has been marked as a duplicate of this bug. ***