Refactor distribution.draw_values #2902
Conversation
… node-inputs. Now the tree dependence is constructed to set the givens dict.
…of dependent variables.
|
After submitting the pull request #2902, I noticed that the travis testsuite was failing. The only test that failed was I changed the test now to check that it is possible to make successful calls to |
pymc3/distributions/distribution.py
Outdated
| @@ -230,16 +230,65 @@ def draw_values(params, point=None): | |||
| """ | |||
| # Distribution parameters may be nodes which have named node-inputs | |||
| # specified in the point. Need to find the node-inputs to replace them. | |||
| givens = {} | |||
|
|
|||
| # Issue #2900 describes a situation in which, the named node-inputs | |||
There was a problem hiding this comment.
This information is too detailed here. It is better to put into the release note.
pymc3/tests/test_random.py
Outdated
| val2 = draw_values([a], point={'sd_log__': np.array([2., 3.])})[0] | ||
| assert np.all(val1 != val2) | ||
| # After #2900 theano.gof.MissingInputError should not be raised | ||
| # with a plain draw_values |
There was a problem hiding this comment.
I dont think we need comments here.
|
@junpenglao, thanks for the comments. I'll fix those parts of the code, but first I'll try to find the bug that is making the tests fail on travis. Apparently it has something to do with |
…ent to RELEASE-NOTES. Fixed bug in the interaction between draw_values, _draw_value and _compile_theano_function. In some cases, draw_values would set an item of the givens dictionary to a theano.tensor.TensorConstant. In _draw_value(param, ...), if param was a theano.tensor.TensorVariable without a random method, and not set in point, _compile_theano_function would be called, using as one of its variables, a theano.tensor.TensorConstant. This lead to TypeError: ("Constants not allowed in param list",...) exceptions being raised. The fix was to skip the inclusion into the givens dictionary of named nodes that were instances of theano.tensor.TensorConstant, because their value would already be available for theano during the function compilation.
…nt, but it occurred on theano.tensor.sharedvar.SharedVariable instances. The error that was raised was similar, SharedVariables cannot be supplied as raw input to theano.function. The fix was the same as for TensorConstants, skip them when constructing the givens dictionary.
|
@junpenglao, I think that I finally routed out the bug that made the tests fail in the CI. The problem was that in some cases, |
…nsorConstant and SharedVariable types, these nodes could be added to the stack again later because their names would not be in givens.keys(). To counter that, a separate set, `stored`, with the names of nodes that are either stored in givens or whos values should be available to theano.function, is used to chose which nodes to add to the stack.
pymc3/distributions/distribution.py
Outdated
| @@ -260,13 +260,17 @@ def draw_values(params, point=None): | |||
| if next_ in givens.keys(): | |||
| # If the node already has a givens value, skip it | |||
| continue | |||
| elif isinstance(next_, theano.tensor.TensorConstant): | |||
| # If the node is a TensorConstant, its value will be | |||
| elif isinstance(next_, tt.TensorConstant) or \ | |||
There was a problem hiding this comment.
isinstance(next_, (tt.TensorConstant, tt.sharedvar.SharedVariable))
|
LGTM, waiting for @junpenglao to merge. |
|
Well done @lucianopaz !!! And congrats on the first contribution ;-) |
|
Thanks! I'm glad I could contribute! |
This solves issue #2900.
Just two things were changed:
distribution.draw_valuesmodel.get_named_nodesThe problem occured in automatically transformed RVs because the corresponding free RV (for example,
awould be theTransformedRVanda_log__theFreeRV) was interpreted as input to theTransformedRVand at the same time, lacked arandommethod. When trying to draw a sample froma, which did definerandom,draw_valuewould raise atheano.gof.fg.MissingInputErrorbecause it first tried to call_draw_valuefora_log__.To cope with this possibility, instead of searching only for the leaf named nodes, like the original
model.get_named_nodesfunction did, we get the leaf nodes, and also the parent/child relations between named nodes. The new function that does this ismodel.get_named_nodes_and_relations.draw_valuenow first tries to_draw_valuefrom the leaf nodes but if this fails, it attempts to do the same on that node's parents. If_draw_valuesucceeds on the parent, its value is passed togivens. The rest of the execution is the same.