-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add tutorial on time series modelling #307
Conversation
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 for the PR! Could you add some assertions/checks as in e.g. https://github.com/TuringLang/TuringTutorials/blob/0e1fa7a385037c59b4f00f0aa521680601472f53/tutorials/00-introduction/00_introduction.jmd#L208? This makes it easier to bump dependencies without checking all results (or having to be familiar with all models and expected outcomes).
I'm not sure why buildkite does not run the tutorial, maybe it doesn't run for PRs from forks @staticfloat?
Indeed. If you want that, go into the buildkite admin settings, under |
Thanks! I enabled it for PRs from forks conditional on a "buildkite" label. |
Thank you for the great suggestions @devmotion, very thorough review! I'm quite busy during the weekend but will address them early next week. |
Thanks again for your comments @devmotion ! I have updated the PR accordingly, with a few exceptions which I motivate in your the relevant comments. I see buildkite is crashing with |
Can you recreate the Manifest file with Julia 1.6? That would fix the issue. |
It stopped converging when running with Julia 1.6. Inference was always very flaky.
Fixed! |
) | ||
end | ||
|
||
βc = group(chain, :βc).value |
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
is an undocumental internal implementation detail. It would be cleaner to work with the resulting chain directly or convert it to an array with Array(...)
.
The model fits! What about the infered cyclic components? | ||
|
||
```julia | ||
βc = group(chain, :βc).value |
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.
Same here.
$$ | ||
|
||
with a Gaussian likelihood $y \sim \mathcal{N}(f(t), \sigma^2)$. | ||
For convenience we are treating the cyclical feature weights $\beta_{\sin{},i} and \beta_{\cos{},i}$ the same in code and weight them with $\beta_c$. |
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.
For convenience we are treating the cyclical feature weights $\beta_{\sin{},i} and \beta_{\cos{},i}$ the same in code and weight them with $\beta_c$. | |
For convenience we are treating the cyclical feature weights $\beta_{\sin{},i}$ and $\beta_{\cos{},i}$ the same in code and weight them with $\beta_c$. |
|
||
decomp = get_decomposition(decomp_model, x, cyclic_features, chain) | ||
decomposed_plt = plot_fit(t, yf, decomp, yf_max) | ||
combined_plt = plot(predictive_plt, decomposed_plt...; layout=(4, 1), size=(700, 1000)) |
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.
Shouldn't this be
combined_plt = plot(predictive_plt, decomposed_plt...; layout=(4, 1), size=(700, 1000)) | |
plot(predictive_plt, decomposed_plt...; layout=(3, 1), size=(700, 1000)) |
?
|
||
decomp = get_decomposition(decomp_model, x, cyclic_features, chain) | ||
decomposed_plt = plot_fit(t, yg, decomp, 0) | ||
combined_plt = plot(predictive_plt, decomposed_plt...; layout=(4, 1), size=(700, 1000)) |
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.
combined_plt = plot(predictive_plt, decomposed_plt...; layout=(4, 1), size=(700, 1000)) | |
plot(predictive_plt, decomposed_plt...; layout=(3, 1), size=(700, 1000)) |
|
||
With the model specified and with a reasonable prior we can now let Turing decompose the time series for us! | ||
|
||
```julia |
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 HTML file is quite large, probably due to the size of some of the SVGs. I assume the problem might be this plot (and the similar one below). Maybe we can enforce png output here by setting chunk options? Eg.,
```julia | |
```julia; fig_ext = ".png" |
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.
many thanks @SebastianCallh!
My pleasure @yebai ! Thank you for your work on Turing! |
Please let me know if you have any comments or suggestions before merging. I could not run weave the file as described in the readme (it just got stuck forever) but I tested all the code in the REPL so it should work.
In the issue I describe working with the air passengers data set, but I couldn't find a nice way to load it into the tutorial so to avoid the problem altogether I opted for synthetic data,
Closes #306.