search

probcomp / metaprob

An embedded language for probabilistic programming and meta-programming.
GNU General Public License v3.0
168 stars 17 forks source link

Examples minor bugs #143

Closed genmeblog closed 5 years ago

genmeblog commented 5 years ago

Hi, I was trying to run examples and there are some minor issues which can be easily fixed:

  1. Some examples do not import infer-and-score (aide or curve-fitting)
  2. There is a parameter bug in earthquake eq-importance-assay: missing :n-particles key
bzinberg commented 5 years ago

Looks like part 1 of https://github.com/probcomp/metaprob/issues/143#issue-433660738 was fixed in #146.

bzinberg commented 5 years ago

@genmeblog, what do you mean by part 2 of https://github.com/probcomp/metaprob/issues/143#issue-433660738? I don't see a place where n-particles is missing, can you link to a line of the code?

genmeblog commented 5 years ago

For part 2 it's here: https://github.com/probcomp/metaprob/blob/master/src/metaprob/examples/earthquake.clj#L166

bzinberg commented 5 years ago

Ah I see. So you're suggesting to change that line to :n-particles n-particles)))), otherwise importance sampling will always use the default value of 1?

genmeblog commented 5 years ago

Well... it's a syntax error. importance-resampling expects map (for associative destructuring). When you call this function in current shape you'll get an execution error (IllegalArgumentException). So yes, this line has to be :n-particles n-particles)))) to make it work.

alex-lew commented 5 years ago

Thanks, @genmeblog.

@bzinberg FYI, this has been fixed since March 30 on the autodiff branch: https://github.com/probcomp/metaprob/commit/ca437c4a5a34d262eeda82713e7abe4e6ca57e81#diff-6b7158b9913b99f823c5cb9670ba6072R166

bzinberg commented 5 years ago

Ah, got it. Thanks @genmeblog, I'm new to Clojure :slightly_smiling_face:

Alex, good to know. Looks like there are a few miscellaneous fixes in that branch which should probably get separate PRs.