Bug in trace.proposeChange?

[longouyang]

A webchurch user found problems with this model:

(mean
 (mh-query
  1000 1
  (define x (if (flip .5) 1 0))

  x

  (if x
      (= (gaussian  10 1 10) 10)
      (= (gaussian -10 1 10) 10))))

This model tries to figure out whether the observation 10 came from N(-10,1) or N(10,1). The model should should strongly prefer N(10, 1) over N(-10,1) but it currently believes that the two Gaussians are equiprobable. This has to do with its use of a relatively untested webchurch feature: supplying a conditioned-value argument to a continuous ERP, e.g., the third argument to (gaussian 10 1 10).

Noah and I tracked the bug down to these lines in RandomExecutionTrace.prototype.proposeChange:

fwdPropLP += nextTrace.newlogprob
rvsPropLP += nextTrace.oldlogprob

Because MH only proposes changes to x and the kernel for flip always proposes switching to the other value (false to true, true to false), the forward and reverse proposal probabilities for this model should always be 1 but aren't. This is because newlogprob and oldlogprob are including the scores of the conditioned variables.

newlogprob is updated in RandomExecutionTrace.prototype.lookup:

var ll = erp.logprob(val, params)
this.newlogprob += ll
if (conditionedValue == undefined) this.genlogprob += ll

Notice that there is also a genlogprob property that only keeps track of variables that were truly generated.

And oldlogprob is updated in RandomExecutionTrace.prototype.traceUpdate:

// Clear out any old random values that are no longer reachable
for(var i=0,rec; rec = oldvarlist[i]; i++) {
  // before checking to see if the old random value is active,
  // make sure that it was not supplanted by a new value
  // (e.g., change of support)
  if( this.vars[rec.name] == rec && !rec.active) {
    this.oldlogprob += rec.logprob
    delete this.vars[rec.name]
  }
}

My proposed fixes are:

1. For the forward proposal probability, use genlogprob rather than newlogprob. I'm not positive that this is the right fix because newlogprob is modified in LARJ (so maybe this change would break inference in LARJ?)
2. Fix the reverse proposal probability by skipping over conditioned ERPs when we compute oldlogprob:

// Clear out any old random values that are no longer reachable
for(var i=0,rec; rec = oldvarlist[i]; i++) {
  // before checking to see if the old random value is active,
  // make sure that it was not supplanted by a new value
  // (e.g., change of support)
  if( this.vars[rec.name] == rec && !rec.active) {
    if (!rec.conditioned) { // proposed fix
      this.oldlogprob += rec.logprob
    }
    delete this.vars[rec.name]
  }
}

I'm more confident about this second fix but it'd be nice to have independent confirmation that this is sane.

Thoughts?