Metropolis Hastings with All or Nothing for Likelihood #54
Conversation
There was a problem hiding this comment.
Awesome; great work, Jingnong!
I left a few focused / detail-oriented comments in the code. One high-level thing I'd like to discuss is: should we make mh_sample more modular / separated out? One thing I can imagine: grammar becomes a folder/package, and then we have a module for inference that defines the algorithms (mainly MH for now).
Similarly: I wonder whether the sampling step and the acceptance step should be factored out and passed in as arguments, instead of "hard-coded" in the same mega-method. What do you think?
|
|
||
| indefinites_grammar = Grammar.from_yaml("indefinites/grammar.yml") | ||
| #indefinites_grammar = Grammar.from_module("indefinites.grammar_functions") | ||
| print(indefinites_grammar.parse("and(not(K+), or(N-, not(SE-)))").hm_sample(indefinites_grammar, [(universe.referents[2], True)])) |
There was a problem hiding this comment.
It's important for the indefinites example to not modify this file directly. The value indefinites_grammar is used in other scripts in the example.
Can you make an example demo-ing the MH sampling elsewhere (either its own file here, or a new sub-folder)?
| if random.random() < mh_accept: | ||
| return(new_tree) | ||
|
|
||
| def prior(self, grammar: "Grammar") -> float: |
There was a problem hiding this comment.
Conceptually, I think I'd vote for prior belonging to Grammar and taking an expression as input (i.e. the grammar is the thing that assigns probabilities to expressions), but am open to being convinced otherwise
| @@ -258,6 +343,9 @@ def add_rule(self, rule: Rule): | |||
| ) | |||
| self._rules_by_name[rule.name] = rule | |||
|
|
|||
| def probability(self, rule: Rule) -> float: | |||
There was a problem hiding this comment.
Probably worth exploring whether we can use cache or cached_property for this, so that it only gets computed once; see https://docs.python.org/3/library/functools.html
No description provided.