Closed NathanielF closed 1 year ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Thanks so much for the contribution so far!
Just set the remote checks running. Will try to review over the weekend, but might drift into early next week.
Thanks. Will have a look at that failing check this evening. Think it is failing on the codespell checks which were skipped for some reason when I ran it locally.
Tried to address the failing codespell check. Seemed to work but not entirely sure.
Ooops, I didn't mean to remove the request for review from @drbenvincent.
Also, didn't mean to rush you @lucianopaz. Just wondering is the "request re-review" button is the right etiquette or would you already have seen the changes i've made since you requested them? Don't mean to put pressure on, just meant to signal that i think i've addressed the above.
Thanks so much for your feedback on this @lucianopaz i will adapt the above discussed and review for grammar and tone. Hopefully push some changes later today.
Ah, sorry @drbenvincent I did the same thing again. No idea why it knocked you out when i requested a review.
Just giving this a slight nudge @lucianopaz , @drbenvincent. Think it's nearly there and it'd be cool if we could get it over the line this week?
Thanks again for your time on this @drbenvincent, I know we're all busy. Happy to wait for @lucianopaz to give the above another look.
I think i've addressed all your comments and his prior comments. In the last commit there i've made the model-diagram neater for all models. I've also stressed that we're adding structural components not for arbitrary reasons but because real-world data tends to have multiple influences... and bayesian structural time-series modelling is one way of capturing theses multi-aspected data generating processes.
For @lucianopaz 's comments above - the main change was to revert to using the coefs pattern rather than having priors for two individual coefficients separately.
Just saw this in a quick look now... In the very final plot, the predicted mean seems not matched up with the shaded regions. Maybe worth a double check.
@drbenvincent do you mean at the tail end of the prediction is seems to come out of phase? Or just about the degree of oscillation? If i change the prior of the beta_fourier terms I get slightly more pronounced oscillation:
I wasn't too concerned about it. The point of the plot was to just show that we can recover the seasonality pattern. I think it does that...
I think it's too much of a rabbit hole to go down, to try and figure out if the percentile color-gradient technique is getting the color map banding exactly right. To my mind the color mapping is there just to suggest that the probabilistic outcomes come with a graded range of plausibility...
It was the phase that I noticed. Thought I'd mention it in case or anything obvious.
Right, yeah... honestly not sure why that is happening.
I added more samples to the plot ,extended the prediction period for longer and allowed a wider sigma on the fourier terms. Still slight phasing visible, but it doesn't seem to be a preface to doom. I don't think it's anything to worry about.
These are great observations @lucianopaz! Thanks for digging into it. I'll adjust and these and re-push today.
Thanks so much again @lucianopaz that last observation about the plotting function was subtle. I've been looking at the function too long to have seen it! I've adjust the notebook in the manner you suggested and indeed was able to recover a prediction plot with the phasing issues:
I added a small note about the AR logic in the comments to the code:
Do you think this is sufficient or should i add anything else?
Thanks @lucianopaz I fixed those last two issues. It was really great working on this issue with yourself and @drbenvincent I learned a tonne!
Fantastic. This has been a great experience. I hope to follow it up shortly with another pull request on the Bayesian VAR models. Thanks again for your time on this!
Forecasting AR Structural Timeseries models
Adding this merge request here to for the open issue: https://github.com/pymc-devs/pymc-examples/issues/450 The notebook has the broad structure of the related blog post and i'm happy to take any feedback or suggestions on streamlining it. I believe it follows the jupyter style advice accurately.
It seems to be passing the pre-commit checks locally. I found it a bit frustrating to get jupytext to pass.
Helpful links