Skip to content

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

Merged
ricardoV94 merged 2 commits intopymc-devs:mainfrom
ricardoV94:grw_ar_init
May 19, 2022
Merged

Use unit normal as default init_dist in GaussianRandomWalk and AR#5779
ricardoV94 merged 2 commits intopymc-devs:mainfrom
ricardoV94:grw_ar_init

Conversation

@ricardoV94
Copy link
Member

Closes #5744

@ricardoV94 ricardoV94 requested review from canyon289 and twiecki May 18, 2022 07:37
@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 requested a review from lucianopaz May 18, 2022 08:12
@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