Skip to content
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

Use unit normal as default init_dist in GaussianRandomWalk and AR #5779

Merged
merged 2 commits into from
May 19, 2022

Conversation

ricardoV94
Copy link
Member

Closes #5744

@ricardoV94 ricardoV94 added this to the v4.0.0 milestone May 18, 2022
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #5779 (c6e58ed) into main (862bd05) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5779      +/-   ##
==========================================
- Coverage   89.27%   89.27%   -0.01%     
==========================================
  Files          74       74              
  Lines       13813    13809       -4     
==========================================
- Hits        12332    12328       -4     
  Misses       1481     1481              
Impacted Files Coverage Δ
pymc/distributions/timeseries.py 77.55% <100.00%> (ø)
pymc/sampling.py 88.34% <0.00%> (-0.06%) ⬇️

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 18, 2022

Should we issue a warning that the default behavior changed from being a Flat in V3 to being a unit Normal?

Edit: Added it to the release notes at least

@ricardoV94 ricardoV94 merged commit 00d4372 into pymc-devs:main May 19, 2022
@twiecki
Copy link
Member

twiecki commented May 19, 2022

Didn't we default to Flat? I think that makes most sense. Or at least a Normal with high SD.

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 20, 2022

@twiecki The problem with flat is that you can't do prior and posterior predictive with it.

You couldn't do this before with AR ofc.

The GRW did not respect the init dist and just assumed the initial point was zero, regardless of what the distribution was. That was just broken behavior imo.

We can make the default Flat, but users will perhaps be upset when their model can't be used for posterior predictive after sampling. I don't have a strong preference.

@twiecki
Copy link
Member

twiecki commented May 23, 2022

That's a good point. How about a SD of 10? I know it's arbitrary, but not more arbitrary than 1. And maybe add a note that it's important for the user to test.

@twiecki
Copy link
Member

twiecki commented May 23, 2022

Hm, but why can't we do posterior predictive with Flat? We still get a non-flat posterior for the initial dist?

@ricardoV94
Copy link
Member Author

Hm, but why can't we do posterior predictive with Flat? We still get a non-flat posterior for the initial dist?

If the GRW is the likelihood, posterior predictive will try to resample it using forward sampling, which it can't because of the Flat. If it's a variable that is in the trace then it won't be an issue as it won't be resampled.

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 23, 2022

We can also just remove the default behavior and force users to always specify the init_dist. That's probably the best if there is no sane default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve default GaussianRandomWalk and AR init distributions
3 participants