Skip to content

Commit

Permalink
Fix bug in RegressionTests.segment
Browse files Browse the repository at this point in the history
  • Loading branch information
@emwap
emwap committed Jan 7, 2015
1 parent 610053e commit e4ffec3
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion tests/RegressionTests.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pmap f = map await . force . map (future . f)

segment :: Syntax a => Data Length -> Pull DIM1 a -> Pull DIM1 (Pull DIM1 a)
segment l xs = indexed1 clen (\ix -> take l $ drop (ix*l) xs)
where clen = length xs `div` l
where clen = 1 + length xs `div` l
-- End one test.

-- | We rewrite `return x >>= \_ -> return y` into `return x >> return y`
Expand Down

4 comments on commit e4ffec3

@pjonsson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case analysis on old code:

length xs < l: clen = 0
length xs == l: clen = 1

Remaining case is length xs > l, but plugging in a concrete number for simplicity:

length xs == l + 1: clen = 1

The old code fails when the length of xs isn't an integer multiple of l. Case analysis on new code:

length xs < l: clen = 1
length xs == l: clen = 2

Remaining case is length xs > l, but plugging in a concrete number for simplicity:

length xs == l + 1: clen = 2

The new code fixes the failing cases. I have written code which does exactly this so I might be the author of the code in question. My reason for writing the code was to experiment with work distribution across cores and I assumed that l would divide the length of xs. Calling segment 2 [1,2] and getting [[1,2], []] as result is semantically correct but not the desired operational behaviour.

@emwap
Copy link
Member Author

@emwap emwap commented on e4ffec3 Jan 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the old code was designed to only work (well) with length xs being a multiple of l. However, since xs is generated by QuickCheck and l is 1024, this is almost never true.

Another fix would be documenting the limitation and constraining the QuickCheck generator.

@emwap
Copy link
Member Author

@emwap emwap commented on e4ffec3 Jan 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand the function is not yet used with a QuickCheck generator...

@pjonsson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code wasn't really designed--it was more of an experiment that I had laying around on the side. I kept running into regressions with that code so I added it to the test suite. Your patched version is semantically correct and catches null vector bugs in plugins so we should just leave it at that.

Please sign in to comment.