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

Get mcmc sampling to work #9

Merged
merged 17 commits into from Jul 6, 2018

Conversation

Projects
None yet
6 participants
@sharanry
Collaborator

sharanry commented Jun 13, 2018

  • Unobserved variables accessible
  • Sampling works

TODO:
Write Tests

@sharanry sharanry requested review from ferrine and removed request for ferrine Jun 13, 2018

@sharanry sharanry changed the title from Get mcmc sampling to work to [WIP] Get mcmc sampling to work Jun 13, 2018

@sharanry

This comment has been minimized.

Show comment
Hide comment
@sharanry

sharanry Jun 13, 2018

Collaborator

@ferrine
What do you suggest model.target_log_prob_fn() give? The logp function of only the result of model.f or all the intermediate RVs too?

If we give it only the final logp function then we wont be able to sample traces of intermediate RVs.
One more problem is tfp.mcmc.sample_chain() might work for only single RV(in current implementation, final) logp function .

And should the model.unobserved contain only final RV, i.e, the result of f()?

Collaborator

sharanry commented Jun 13, 2018

@ferrine
What do you suggest model.target_log_prob_fn() give? The logp function of only the result of model.f or all the intermediate RVs too?

If we give it only the final logp function then we wont be able to sample traces of intermediate RVs.
One more problem is tfp.mcmc.sample_chain() might work for only single RV(in current implementation, final) logp function .

And should the model.unobserved contain only final RV, i.e, the result of f()?

@sharanry sharanry self-assigned this Jun 13, 2018

@ferrine

Review

Graph namespace

TF uses graph.as_default() to create graph. So if you repeatedly call model.unobserved it will spoil the namespace totally. The below snippet can replicate the problem

import tensorflow as tf
graph = tf.get_default_graph()
sess = tf.InteractiveSession(graph=graph)
def model():
    return tf.ones([1])
model()
model()
model()
graph.as_graph_def()

The output contains a lot of versions of tf.ones(). One way to solve the problem it to put all internal things into an auxiliary namespace.

Show outdated Hide outdated pymc4/inference/sampling/sample.py Outdated
@ferrine

There is a special interceptor for this purpose

Show outdated Hide outdated pymc4/model/base.py Outdated
Show outdated Hide outdated pymc4/model/base.py Outdated
Show outdated Hide outdated pymc4/model/base.py Outdated
@ColCarroll

Just a few styling nitpicks around dictionary iteration!

Show outdated Hide outdated pymc4/inference/sampling/sample.py Outdated
Show outdated Hide outdated pymc4/model/base.py Outdated
Show outdated Hide outdated pymc4/model/base.py Outdated
@@ -9,7 +9,7 @@
'CollectLogProb'
]
VariableDescription = collections.namedtuple('VariableDescription', 'Dist,shape')
VariableDescription = collections.namedtuple('VariableDescription', 'Dist,shape,rv')

This comment has been minimized.

@ferrine

ferrine Jun 26, 2018

Member

I'm still worried about this solution. Variable description is supposed to be collected far before sampling (or what about changing this?). So when you first collect VariableInfo for the first time and get these RVs, you save temporary nodes. When you collect LogProb you again run the model and variables involves there are totally different from those that are stored in VariableDescription. That's why I did not store them there.

@ferrine

ferrine Jun 26, 2018

Member

I'm still worried about this solution. Variable description is supposed to be collected far before sampling (or what about changing this?). So when you first collect VariableInfo for the first time and get these RVs, you save temporary nodes. When you collect LogProb you again run the model and variables involves there are totally different from those that are stored in VariableDescription. That's why I did not store them there.

This comment has been minimized.

@junpenglao

junpenglao Jun 26, 2018

Member

Agree with @ferrine - The RVs are not initialized until we configure the model (following the idea in the API discussion doc, we create the RVs when we call model.configure(...) or model.sample(...)). This means that we record the Distribution and the relationship between RVs, but the actually RVs are only initialized when we actually using them (ie, in the evaluation of logp).

@junpenglao

junpenglao Jun 26, 2018

Member

Agree with @ferrine - The RVs are not initialized until we configure the model (following the idea in the API discussion doc, we create the RVs when we call model.configure(...) or model.sample(...)). This means that we record the Distribution and the relationship between RVs, but the actually RVs are only initialized when we actually using them (ie, in the evaluation of logp).

This comment has been minimized.

@sharanry

sharanry Jun 26, 2018

Collaborator

The problem i am facing if RVs are not stored in the VariableDescription is that it doesn't store the specifics of any distribution (like loc or scale) even if they are mentioned in the model definition. So we will have to collect all this already provided info somehow.

@model.define
def process():
    mu = ed.Normal(loc=0., scale=10., name="mu")
    # here we lose the info that it has loc 0 and scale 10 without RV.

We can try defining a new Interceptor which does this for us for each RV.
We can then overwrite(replace existing and add new) the collected data every-time we call configure.

@sharanry

sharanry Jun 26, 2018

Collaborator

The problem i am facing if RVs are not stored in the VariableDescription is that it doesn't store the specifics of any distribution (like loc or scale) even if they are mentioned in the model definition. So we will have to collect all this already provided info somehow.

@model.define
def process():
    mu = ed.Normal(loc=0., scale=10., name="mu")
    # here we lose the info that it has loc 0 and scale 10 without RV.

We can try defining a new Interceptor which does this for us for each RV.
We can then overwrite(replace existing and add new) the collected data every-time we call configure.

This comment has been minimized.

@ferrine

ferrine Jul 4, 2018

Member

OK, let's make it later

@ferrine

ferrine Jul 4, 2018

Member

OK, let's make it later

@ferrine

This comment has been minimized.

Show comment
Hide comment
@ferrine

ferrine Jun 30, 2018

Member

Hmm, I would avoid mutables in defaults. I think some more generic solution is needed but is not of high priority now

Typing via phone, this refers to the above line 131

Member

ferrine commented on tests/test_model.py in ca9f334 Jun 30, 2018

Hmm, I would avoid mutables in defaults. I think some more generic solution is needed but is not of high priority now

Typing via phone, this refers to the above line 131

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 1, 2018

@ferrine just helping you repeat the comment above.

ericmjl replied Jul 1, 2018

@ferrine just helping you repeat the comment above.

@ferrine

This comment has been minimized.

Show comment
Hide comment
@ferrine

ferrine Jun 30, 2018

Member

This is overtesting, if you inherit from a class, no need to test it

Member

ferrine commented on tests/test_model.py in ca9f334 Jun 30, 2018

This is overtesting, if you inherit from a class, no need to test it

@ferrine

This comment has been minimized.

Show comment
Hide comment
@ferrine

ferrine Jun 30, 2018

Member

This test is not needed, instantiating is explicit. That is the same as checking if attribute is set after setting it

Member

ferrine commented on tests/test_model.py in ca9f334 Jun 30, 2018

This test is not needed, instantiating is explicit. That is the same as checking if attribute is set after setting it

@ferrine

This comment has been minimized.

Show comment
Hide comment
@ferrine

ferrine Jun 30, 2018

Member

That is a good one

Member

ferrine commented on tests/test_model.py in ca9f334 Jun 30, 2018

That is a good one

@ericmjl

This comment has been minimized.

Show comment
Hide comment
@ericmjl

ericmjl Jul 1, 2018

from @ferrine

Hmm, I would avoid mutables in defaults. I think some more generic solution is needed but is not of high priority now

ericmjl commented on tests/test_model.py in ca9f334 Jul 1, 2018

from @ferrine

Hmm, I would avoid mutables in defaults. I think some more generic solution is needed but is not of high priority now

@sharanry sharanry changed the title from [WIP] Get mcmc sampling to work to Get mcmc sampling to work Jul 1, 2018

@sharanry

This comment has been minimized.

Show comment
Hide comment
@sharanry

sharanry Jul 3, 2018

Collaborator

@ferrine Could I merge this PR?

Collaborator

sharanry commented Jul 3, 2018

@ferrine Could I merge this PR?

Show outdated Hide outdated pymc4/model/base.py Outdated
@@ -9,7 +9,7 @@
'CollectLogProb'
]
VariableDescription = collections.namedtuple('VariableDescription', 'Dist,shape')
VariableDescription = collections.namedtuple('VariableDescription', 'Dist,shape,rv')

This comment has been minimized.

@ferrine

ferrine Jul 4, 2018

Member

OK, let's make it later

@ferrine

ferrine Jul 4, 2018

Member

OK, let's make it later

assert len(model.observed) == 1
assert not model.unobserved
model.reset()

This comment has been minimized.

@ferrine

ferrine Jul 4, 2018

Member

We decided to meke a copy of model each time state changes

@ferrine

ferrine Jul 4, 2018

Member

We decided to meke a copy of model each time state changes

This comment has been minimized.

@ferrine

ferrine Jul 4, 2018

Member

this one is not critical though, refactoring interceptor usage is what is really needed to finish this PR (#9 (diff))

@ferrine

ferrine Jul 4, 2018

Member

this one is not critical though, refactoring interceptor usage is what is really needed to finish this PR (#9 (diff))

@sharanry

This comment has been minimized.

Show comment
Hide comment
@sharanry

sharanry Jul 6, 2018

Collaborator

@ferrine I have changed it to a class based interceptor

Collaborator

sharanry commented Jul 6, 2018

@ferrine I have changed it to a class based interceptor

@ferrine

I can't find sampling test, I think we need one.

@ferrine

This comment has been minimized.

Show comment
Hide comment
@ferrine

ferrine Jul 6, 2018

Member

And test point is better get via model.test_point()

Member

ferrine commented Jul 6, 2018

And test point is better get via model.test_point()

@sharanry

This comment has been minimized.

Show comment
Hide comment
@sharanry

sharanry Jul 6, 2018

Collaborator

@ferrine Currently model.test_point() is how you get the test point.
I am not sure I understand what you are saying.

Collaborator

sharanry commented Jul 6, 2018

@ferrine Currently model.test_point() is how you get the test point.
I am not sure I understand what you are saying.

num_results=5000,
num_burnin_steps=3000,
step_size=.4,
num_leapfrog_steps=3,
numpy=True):
initial_state = []
for name, shape in model.unobserved.iteritems():
for name, (_, shape, _) in model.unobserved.items():

This comment has been minimized.

@ferrine

ferrine Jul 6, 2018

Member
for name, point in model.test_point(mode=mode):
    initial_state.append(point)
@ferrine

ferrine Jul 6, 2018

Member
for name, point in model.test_point(mode=mode):
    initial_state.append(point)

This comment has been minimized.

@ferrine

ferrine Jul 6, 2018

Member

may be done in next PR

@ferrine

ferrine Jul 6, 2018

Member

may be done in next PR

@ferrine

ferrine approved these changes Jul 6, 2018

@ColCarroll ColCarroll merged commit 4c8d0d5 into pymc-devs:functional Jul 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on unobserved at 84.615%
Details
@ColCarroll

This comment has been minimized.

Show comment
Hide comment
@ColCarroll

ColCarroll Jul 6, 2018

Member

Congrats @sharanry , and thanks for the thorough review @ferrine !

Member

ColCarroll commented Jul 6, 2018

Congrats @sharanry , and thanks for the thorough review @ferrine !

@gammadistribution

This comment has been minimized.

Show comment
Hide comment
@gammadistribution

gammadistribution commented on pymc4/model/base.py in ca9f334 Sep 20, 2018

This comment has been minimized.

Show comment
Hide comment
@ferrine

ferrine Sep 21, 2018

Member

Hi, thanks for a great article. I will take it in account, hope I do not have such corner cases here (seems like I do not)

Member

ferrine replied Sep 21, 2018

Hi, thanks for a great article. I will take it in account, hope I do not have such corner cases here (seems like I do not)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment