-
-
Notifications
You must be signed in to change notification settings - Fork 111
add a draft for traces #139
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
Changes from all commits
8361ae1
6fe8035
3547133
3541d1d
fa30215
4b3ecb6
7b144fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,14 @@ | ||
| from typing import Optional | ||
| import numpy as np | ||
|
|
||
| from pymc4 import Model, flow | ||
| from .. import Model, flow | ||
|
|
||
|
|
||
| def initialize_state(model: Model, observed: Optional[dict] = None) -> flow.SamplingState: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe refine the type declaration here? I.e., change to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is nothing special about observed except keys are strings. The API is not that narrow at this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would be |
||
| """ | ||
| Initilize the model provided state and/or observed variables | ||
| Initilize the model provided state and/or observed variables. | ||
|
|
||
| Parameters | ||
| Parameters | ||
| ---------- | ||
| model : pymc4.Model | ||
| observed : Optional[dict] | ||
|
|
@@ -18,3 +19,26 @@ def initialize_state(model: Model, observed: Optional[dict] = None) -> flow.Samp | |
| """ | ||
| _, state = flow.evaluate_model_transformed(model, observed=observed) | ||
| return state.as_sampling_state() | ||
|
|
||
|
|
||
| def trace_to_arviz(pm4_trace, pm4_sample_stats): | ||
| """ | ||
| Tensorflow to Arviz trace convertor. | ||
|
|
||
| Convert a PyMC4 trace as returned by sample() to an ArviZ trace object | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would change this to to an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure, this is a bit too specific. Let me try to see if I can add a helper class in TFP so that the output is a bit more standardized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add types (including return type)? This information seems to be available in the docstrings... |
||
| that can be passed to e.g. arviz.plot_trace(). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| pm4_trace : dict | ||
|
|
||
| Returns | ||
| ------- | ||
| arviz.data.inference_data.InferenceData | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be shortened to |
||
| """ | ||
| import arviz as az | ||
|
|
||
| posterior = {k: np.swapaxes(v.numpy(), 1, 0) for k, v in pm4_trace.items()} | ||
| sample_stats = {k: v.numpy().T for k, v in pm4_sample_stats.items()} | ||
| sample_stats['tree_size'] = np.diff(sample_stats['tree_size'], axis=1) | ||
| return az.from_dict(posterior=posterior, sample_stats=sample_stats) | ||
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.
being picky here, should we use relative imports for inside the library? Not part of this PR but just wanted to ask
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 use doctest to test code snippets in documentation. Doctest is complaining if import is relative sometimes
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 had that problem with pytest, too -- complaints about relative imports -- and it turned out for me it was because I was running the tests (this is for PyMC3) inside the source directory. When I ran it "above" the directory (i.e., from
pymc3instead ofpymc3/pymc3/) the complaints about relative imports went away.I think the advantage of relative imports is that they don't risk an import cycle as much as absolute ones do.
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 believe this part is true, that relative imports risk less that another package of same name will be imported from
sys.pathversus the adjacent moduleThere 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.
https://stackoverflow.com/a/4209771
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.
My personal feeling is that it is hard to run into a problem you describe, you would not import pymc4 in the first place. But for our use case we can test code snippets in docs without mess
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.
No worries, for this PR just ignore me there's more important things :) thanks @ferrine