Possible mh bug

[bergen]

The following code errors when using mh-query but not when using enumeration-query. Also, the code doesn't error with probability 1: it will usually execute when drawing a small number of samples (e.g. mh-query 1 1).

(define (my-iota n)
  (define (helper n x)
    (if (= x n)
        '()
        (pair x (helper n (+ x 1)))))
  (helper n 0))

(define (my-sample-integer n)
  (uniform-draw (my-iota n)))

(define states (list (list 1)
                     (list 1 2)))

(define (draw-sample y)
  (+ 1 (my-sample-integer (length y))))

(define q
  (mem (lambda () 
         (mh-query 100 1
           (define y (uniform-draw states))
           (define z (draw-sample y))
           z
           #t))))

(q)

Error:

21:4-21:4: argument "undefined" to + is not a number
Stack trace: 21:4-21:4,28:23-28:33

[ngoodman]

A much simpler example for the same bug:

(mh-query 100 1
           (+ 0 (uniform-draw (if (flip) '(0) '(0 1))))
           #t)

Explanation for bug: uniformDraw in probjs is a wrapper around multinomial which samples index then looks up value in list. When domain changes the logprob is -Infinity (eventually leading to rejection) but the return value is same. Since this index is no longer in list, uniformDraw returns undefined. The argument assertion in church plus catches this and throws an error.

In some sense this is the right behavior for all the pieces... One solution is to have probjs bail out of traceupdate as soon as score is -Infinity.

[ngoodman]

Also, if the built in sample-integer is used in the original example there is no bug.

[longouyang]

(note that sample-integer used to be buggy but no longer is)

[stuhlmueller]

I ran into the same problem here.

In some sense this is the right behavior for all the pieces... One solution is to have probjs bail out of traceupdate as soon as score is -Infinity.

This should happen.

[longouyang]

I just discussed this with Noah; there appear to be tradeoffs with any solution.

We should probably all meet in person to hash this out.

[longouyang]

(For the sake of documentation: Andreas, Noah, and I met to discuss this issue. We settled on having each ERP in probjs optionally provide a method for determining whether different parameters give rise to supports that are compatible)

Hopefully fixed in db61bc5fb5e753f4626b526b06c386a623c96fb6

Andreas, let me know if you run into any bugs as you adapt the Forest models that were hampered by this bug

[longouyang]

My support-comparison code apparently induces a bug in nextEnumState inside probjs's trace.js; record lookup can fail if you change a random variable that causes previous addresses to be meaningless (in particular, the first test in the Occam's Razor test file was failing)

I don't totally understand the error, but I think that ignoring failed record lookups inside nextEnumState works: https://github.com/dritchie/probabilistic-js/commit/476f11b525444dac93229881896afeb7eb745d60

But maybe this ought to get fixed in traceUpdate... Noah, could you weigh in on this?

[longouyang]

A simple test case that reveals the (one?) problem caused by support-comparison:

(define bags '([a]
               [a b]
               [a b c]
               [a b c d]))
(define dist
  (enumeration-query
   (define bag (uniform-draw bags)) 
   (define size (length bag))
   (define observations (repeat 3 (lambda () (uniform-draw bag)))) 

   size 

   true))
(barplot dist)

We want a uniform distribution (since we're not conditioning on anything), but the output distribution highly favors smaller bags. This problem disappears if we comment out the observations line. It also disappears if we disable support comparison, but we want support comparison (or a working version, anyway).

[stuhlmueller]

Do you understand why this record lookup change was necessary, and that it is not the source of the problems with enumeration-query?

[longouyang]

The existence of that "continue" line is a symptom of the problem, not its cause.

The cause is the compareSupport line in RandomExecutionTrace.prototype.lookup

With support comparison, record lookups (namely, this.getRecord(varname), see trace.js) will sometimes come up undefined, and then the nextEnumState will try to access properties of that undefined value, immediately leading to an error. This is why I added the "continue" line.

If you don't have support comparison, nextEnumState doesn't seem to suffer from this issue - this.getRecord(varname) always gives you a usable record.

I'm currently spelunking through this, trying to understand why this is.

[longouyang]

Fixed in 6aafd3276fcc65f73e2f07654927602e951ca59b

The cause was that traceUpdate was being too eager in deleting the old random variables recorded in oldvarlist. The compareSupport code was replacing an old rv with a new one when support comparison failed, but the way it did this didn't manage to change the record inside oldvarlist

The relevant fix inside probjs was: dritchie/probabilistic-js@30383de4bb98323a0b36ac571b56496c2fbaccad