-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix fromListWithDef
divergence
#42
Conversation
I'm not quite sure about this approach. The existing implementation is better for finite lists, because Could it be possible to rewrite |
Sounds good, it will take me a little while to figure out. I'll try to finish it by the end of the day. |
Actually I think I can optimize it a little more. I'll fix it shortly, my apologies. |
@Bodigrim Ok, it's ready now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, could you please add a test?
src/Data/Chimera/Internal.hs
Outdated
Left l -> | ||
if l == kk | ||
then (G.replicate kk a, []) | ||
else (G.fromListN kk (xs ++ replicate l a), []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably write it as (if l == kk then ... else ..., [])
, but up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I rewrote it that way
fromListWithDef diverges when passed an infinite list, for the following reasons: Reason 1: * fromListWithDef calls Data.Primitive.Array.fromListN (bits + 1). fromListN requires that the length of the list that is passed to it is exactly bits + 1. To ensure this, it forces the spine of the list that is passed to it. * The list that is passed to fromListN is generated by go0, which calls go. When xs is infinite, go k xs will be infinite. Therefore, fromListN will throw an error since go0 yields a list longer than bits + 1. Even if we take the first bits + 1 elements of go0 before passing to fromListN, there is another problem. Reason 2: * As fromListN forces the spine, it will compute go 0 xs0, go 1 xs1, ..., go 63 xs63, for some xs0, xs1, ..., xs63. * This in turn requires computing measureOff (2^0) xs0, measureOff (2^1) x1, ..., measureOff (2^63) x63, which is essentially divergent. To avoid these problems, we * Add a check in go to break recursion and ensure that the list that is passed to fromListN has length (bits + 1). * Rearrange how we compute go, so that measureOff is not forced while forcing the spine of go.
I added a test for infinite inputs to |
Thanks a ton! |
fromListWithDef
diverges when passed an infinite list, for the following reasons:Reason 1:
fromListWithDef
callsData.Primitive.Array.fromListN (bits + 1)
.fromListN
requires that the length of the list that is passed to it is exactlybits + 1
. To ensure this, it forces the spine of the list that is passed to it.fromListN
is generated bygo0
, which callsgo
. Whenxs
is infinite,go k xs
will be infinite. Therefore,fromListN
will throw an error sincego0
yields a list longer thanbits + 1
.Even if we take the first
bits + 1
elements ofgo0
before passing tofromListN
, there is another problem.Reason 2:
As
fromListN
forces the spine, it will computego 0 xs0
,go 1 xs1
, ...,go 63 xs63
, for somexs0
,xs1
, ...,xs63
.This in turn requires computing
measureOff (2^0) xs0
,measureOff (2^1) x1
, ...,measureOff (2^63) x63
, which is essentially divergent.To avoid these problems, we implementfromListWithDef
in terms offromInfinite
. Note that the implementation offromInfinite
is similar to the old implementation offromListWithDef
, except that it doesn't callmeasureOff
, which is the source of the divergence.Updated solution: To avoid these problems, we
Add a check in
go
to break recursion and ensure that the list that is passed tofromListN
has lengthbits + 1
.Rearrange how we compute
go
, so thatmeasureOff
is not forced while forcing the spine ofgo
.This PR depends on PR #41, which should be merged first.
Fixes issue #39