hakaru-dev / hakaru

A probabilistic programming language
BSD 3-Clause "New" or "Revised" License
311 stars 30 forks source link

Exp scale closure #154

Closed mkhattab940 closed 6 years ago

mkhattab940 commented 6 years ago

Test for exponential distribution closure by scale. Fails with following logs:

### Failure in: 6:RoundTrip:6:0:t_exponential_scale_closure:0
haskell/Tests/TestTools.hs:130
expected:
exponential = fn alpha prob:
              X <~ uniform(nat2real(0), nat2real(1))
              return -prob2real(nat2prob(1))
                      * prob2real(alpha)
                      * log(real2prob(X))
exponentialScaled = fn alpha prob:
                    fn k prob:
                    X <~ exponential(alpha * k)
                    return X
exponentialScaled(1/2, 2/1)
but got:
X3 <~ uniform(+0/1, +1/1)
return log(real2prob(X3)) * (-1/1)
Cases: 330  Tried: 281  Errors: 2  Failures: 15

### Failure in: 6:RoundTrip:6:0:t_exponential_scale_closure:1
haskell/Tests/TestTools.hs:130
expected:
exponential = fn alpha prob:
              X <~ uniform(nat2real(0), nat2real(1))
              return -prob2real(nat2prob(1))
                      * prob2real(alpha)
                      * log(real2prob(X))
exponentialScaled = fn alpha prob:
                    fn k prob:
                    X <~ exponential(alpha * k)
                    return X
exponentialScaled(1/2, 2/1)
but got:
X3 <~ uniform(+0/1, +1/1)
return log(real2prob(X3)) * (-1/1)
Cases: 330  Tried: 282  Errors: 2  Failures: 16
JacquesCarette commented 6 years ago

This is a "good" failure, in the sense that what you got was correct and much simpler than what you expected. You should not have 'expected' that answer in the specific test you tried, as it specializes so much. Fix this, then I can merge.

JacquesCarette commented 6 years ago

Are you going to be working on this soon?

mkhattab940 commented 6 years ago

Sorry for the delay, I'm looking at it now.

I'm really unsure how to go about fixing this as I'm unsure what is incorrect about my expected file that causes it to specialize so much. The two files are virtually the same other than where the scaling factor is placed. I tried switching the names, but the problem persists.

Should I try it with different parameters?

JacquesCarette commented 6 years ago

The last line of your expected file is exponentialScaled(1/2, 2/1). That's just a call, so it fully inlines.

And of course

exponentialScaled = fn alpha prob:
                fn k prob:
                X <~ exponential(alpha * k)
                return X

is just exponentialScaled = fn alpha prob: fn k prob: exponential(alpha * k) and since alpha and k are known here too, that just inlines into exponential. You can do that by hand. You'll get something that ought to be close to what Simplify gets.

mkhattab940 commented 6 years ago

OK, so I've inlined everything in the expected file. It still fails:

### Failure in: 6:RoundTrip:8:0:t_exponential_scale_closure:0
haskell/Tests/TestTools.hs:130
expected:
X <~ uniform(+0/1, +1/1)
return (-1/2) * log(2/1 * real2prob(X))
but got:
X3 <~ uniform(+0/1, +1/1)
return log(real2prob(X3)) * (-1/2) + log(2/1) * (-1/2)
Cases: 338  Tried: 289  Errors: 2  Failures: 22

### Failure in: 6:RoundTrip:8:0:t_exponential_scale_closure:1
haskell/Tests/TestTools.hs:130
expected:
X <~ uniform(+0/1, +1/1)
return (-1/2) * log(2/1 * real2prob(X))
but got:
X3 <~ uniform(+0/1, +1/1)
return log(real2prob(X3)) * (-1/1)
Cases: 338  Tried: 290  Errors: 2  Failures: 23
mkhattab940 commented 6 years ago

OK, I've redone this with the new implementation of exponential, and I've managed to resolve the issue. The failure now looks like this:

### Failure in: 6:RoundTrip:0:0:t_exponential_scale_closure:1
haskell/Tests/TestTools.hs:130
expected:
gamma(1/1, 3/2)
but got:
X3 <~ gamma(1/1, 1/2)
return X3 * (3/1)
Cases: 133  Tried: 85  Errors: 0  Failures: 1
JacquesCarette commented 6 years ago

This is a bit odd, but it's something I should look into. I think what you've coded is correct, and this may be a weakness in Hakaru, I'm not sure. But I should be able to merge this in now.