Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#321 closed defect (fixed)

build_numeric_ranges sometimes builds invalid ranges

Reported by: shane Owned by: Richard Boulton
Priority: normal Milestone: 1.1.4
Component: Library API Version: SVN trunk
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

CategorySelectMatchSpy::build_numeric_ranges can occasionally construct ranges where one interval overlaps all others.

The attached python script shows the problem (apologies for the external dependency). It prints the range constructed:

[((11.949999999999999, 60.0), 2), ((14.5, 14.5), 1)]

when it should be:

[((11.949999999999999, 11.949999999999999), 1), ((14.5, 14.5), 1), ((60.0, 60.0), 1)]

This was using fedora 10 on x86_64.

The attached patch fixes this.

b was getting set to size_t(-1) due to taking the floor of a number that was slightly on the wrong side of 0. It then later got set to n_buckets and that resulted in items that should have been in the smallest bucket ending up in the largest one.

Attachments (2)

matchspy_category_underflow.patch (500 bytes ) - added by shane 15 years ago.
facet_error.py (628 bytes ) - added by shane 15 years ago.

Download all attachments as: .zip

Change History (9)

by shane, 15 years ago

Attachment: facet_error.py added

comment:1 by Olly Betts, 15 years ago

Component: OtherLibrary API
Version: SVN trunk

I don't think floor -> round is the correct fix here - that will put values in the wrong bucket.

The variable lo should be the lowest value, and:

start = floor(lo / unit) * unit;

And unit should be positive.

So start should be strictly less than any value being considered...

If it was x86 I'd suspect excess precision, but you say it's x86-64.

I've added some assertions to try to narrow down what is going wrong.

comment:2 by Olly Betts, 15 years ago

Um, not strictly less - I meant less than or equal...

comment:3 by shane, 15 years ago

Ah, ok, I understand why you're rounding down.

It seems that sometimes start > lo due to fp rounding.

In my example, I have:
lo = 11.949999999999999289457264239899814128875732421875
unit = 0.01000000000000000020816681711721685132943093776702880859375
and get:
start = 11.9500000000000010658141036401502788066864013671875

This also happens on Opteron.

I guess the solution is:

if (start > lo) start = lo;

comment:4 by Olly Betts, 15 years ago

Status: newassigned

Yes, that fix seems good.

I'll it to trunk.

It would be nice to have a regression test for this that doesn't require xappy - should be easy enough now we know what's going on I think - but this code isn't actually currently even built used on trunk...

comment:5 by Olly Betts, 14 years ago

Owner: changed from Olly Betts to Richard Boulton
Status: assignednew

Richard confirms this is definitely fixed on trunk and there's a regression test in xappy's testsuite.

He said on IRC he'd check if there was a xapian test for it, so reassinging.

comment:6 by Richard Boulton, 14 years ago

Resolution: fixed
Status: newclosed

Added a Xapian test in r13658, which passes with trunk, but fails if the fix (from r11812) is reverted.

Closing this, therefore. Not marking it as applying to a particular milestone, since when it was actually fixed, it applied to the matchspy branch, and the fix was included before the matchspy features were added to trunk.

comment:7 by Olly Betts, 14 years ago

Milestone: 1.1.4

I think "fixed in 1.1.4" is probably appropriate since you just committed a regression test for it which will debut there.

Note: See TracTickets for help on using tickets.