Bug 2607 - Fixing "X-Spam-Status: No, hits=5.0 required=5.0"
Summary: Fixing "X-Spam-Status: No, hits=5.0 required=5.0"
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 2.60
Hardware: All All
: P4 minor
Target Milestone: 3.0.0
Assignee: Malte S. Stretz
URL: http://wiki.spamassassin.org/w/Roundi...
Whiteboard:
Keywords:
: 3229 3757 (view as bug list)
Depends on:
Blocks: 2498 2659
  Show dependency tree
 
Reported: 2003-10-18 15:31 UTC by Dan Kohn
Modified: 2004-09-07 19:46 UTC (History)
3 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
Proposed patch file patch None Dan Kohn [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Kohn 2003-10-18 15:31:02 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.
Comment 1 Dan Kohn 2003-10-26 11:09:42 UTC
Created attachment 1509 [details]
Proposed patch file

Here's a patch that implements my proposed change to fix the confusing rounding
info.
Comment 2 Duncan Findlay 2004-01-01 18:00:25 UTC
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
Comment 3 Malte S. Stretz 2004-01-05 13:38:43 UTC
Seems like everybody agrees in the end :) Dan, do we have a CLA from you? 
(Bugzilla doesn't think so.) 
Comment 4 Dan Kohn 2004-01-05 15:50:16 UTC
I just faxed in my CLA.
Comment 5 Malte S. Stretz 2004-02-15 17:06:18 UTC
I think Dan's right, this can go into 2.6x. Can somebody please confirm his 
CLA has arrived? 
Comment 6 Theo Van Dinter 2004-02-15 17:27:12 UTC
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. :)
Comment 7 Justin Mason 2004-02-17 10:28:40 UTC
>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.
Comment 8 Justin Mason 2004-02-17 10:29:28 UTC
oh, +1 to Dan's fix too
Comment 9 Malte S. Stretz 2004-02-18 16:32:31 UTC
Committed to HEAD but decided against backporting. 
Comment 10 Theo Van Dinter 2004-03-30 08:48:33 UTC
*** Bug 3229 has been marked as a duplicate of this bug. ***
Comment 11 Malte S. Stretz 2004-09-08 03:46:30 UTC
*** Bug 3757 has been marked as a duplicate of this bug. ***