-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix _check_start_shape
and BinaryMetropolis.astep
#4698
Conversation
@@ -278,7 +277,7 @@ def test_run(self): | |||
"theta": np.array(5, dtype="f"), | |||
} | |||
step_one = pm.Metropolis([model["theta_interval__"], model["psi_logodds__"]]) | |||
step_two = pm.BinaryMetropolis([model.z]) | |||
step_two = pm.BinaryMetropolis([model.rvs_to_values[model["z"]]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to pass the value_var
explicitly, or it works as well with the rv_var
(i.e., model["z"]
)?
The test passes but I didn't have the patience to check if it affected sampling.
The ArrayStep
docs are ambiguous in this regard:
Blocked step method that is generalized to accept vectors of variables.
Parameters
----------
vars: list
List of variables for sampler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value variables are given to the step methods. See here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant when specifying manually the steps as in this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, step methods require value variables as their vars
argument/field, so it looks like your manual setup for that step method in the test is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that was exactly my hunch.
Should we add some logic where we check if a variable manually assigned to a stepper contains a value_var
and if so use that instead?
That would make manual assignments less cumbersome and error prone. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to state what kind of variables they should be in a docstring. Aside from that, we could perhaps check that the vars
are actually value variables in the step method's model
.
090d7f4
to
19dcc45
Compare
Thanks @ricardoV94! |
This PR fixes two small issues in the methods mentioned in the PR title.
Two tests are now failing because they end in a call to
az.summary
, is this known to still be broken?