Conversation
|
@avehtari There's not much to review here. I requested your review just to confirm that you definitely want to do this and to make sure the documentation is ok. The code change is just |
|
I added one suggestion. Usually, people check reasonable convergence and ESS before running |
|
This is passing everything now, but wanted to check with @andrjohns before merging this: if we're changing a default argument would you prefer that I add a warning or message if the user calls the loo method without changing the new default? On the one hand, I guess this could be considered a breaking change (although wouldn't lead to any errors it changes default behavior). On the other hand, adding a warning/message would result in a warning or message most of the times loo() is called, which is kind of annoying. And this is a pretty small change and it's documented and we can put it in the release notes. I'm a bit torn on whether we should add a warning/message. @andrjohns any preference? |
Submission Checklist
Summary
Changes default from
TRUEtoFALSEfor ther_effargument to theloo()method. This is based on @avehtari's comment at https://discourse.mc-stan.org/t/loo-subsample-slower-than-loo/39556/5Copyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Columbia University
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: