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

Updates for Regression and Other Stories #432

Merged
merged 27 commits into from
Jul 8, 2020
Merged

Updates for Regression and Other Stories #432

merged 27 commits into from
Jul 8, 2020

Conversation

jgabry
Copy link
Member

@jgabry jgabry commented Apr 27, 2020

@bgoodri and others: this PR isn't ready to merge yet because it's still missing changes to the default prior mean (due to issues in sorting out how to do that coherently). So far I have just changed the scaling so that:

  • autoscale defaults to FALSE in functions like normal(), student_t(), etc.
  • but autoscale is set to TRUE when those functions are used for default priors so that default priors continue to get autoscaled like before.
  • The default prior scale for the intercept before autoscaling is 2.5 instead of 10, which was deemed to be way too wide after autoscaling

This means that informative priors don't require explicitly setting autoscale=FALSE, as requested by @andrewgelman and @avehtari for RAOS.

* default to FALSE in functions for setting priors
* explicitly set to TRUE in default prior usage to keep autoscaling by default
@jgabry
Copy link
Member Author

jgabry commented Apr 27, 2020

Ok, I've also now added a preliminary version of default_prior_intercept() and default_prior_coef()

R/priors.R Outdated
@@ -562,7 +561,22 @@ R2 <- function(location = NULL, what = c("mode", "mean", "median", "log")) {
list(dist = "R2", location = location, what = what, df = 0, scale = 0)
}

#' @rdname priors
#' @export
default_prior_intercept = function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think default_prior_* should take a family argument and then stan_glm, etc. should default to default_prior(family). This will give us more flexibility to change things in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. What about for functions like stan_lmer(), stan_glm.nb(), stan_glmer.nb(), where there's no family argument?

That is, for stan_glmer() we can have

stan_glmer(..., prior_intercept = default_prior_intercept(family))

but for stan_lmer() and others should we just fill in the family like this?

stan_lmer(..., prior_intercept = default_prior_intercept(family=gaussian()))
stan_glmer.nb(..., prior_intercept = default_prior_intercept(family=neg_binomial_2()))
stan_glm.nb(..., prior_intercept = default_prior_intercept(family=neg_binomial_2()))

Or some other option?

@jgabry
Copy link
Member Author

jgabry commented Apr 28, 2020

Comments from discussion:

  • add a slot to returned list that includes version number
  • add a slot for the modeling function name

we will have to make this a method of a S3 generic function at some point
@jgabry
Copy link
Member Author

jgabry commented May 11, 2020

we will have to make this a method of a S3 generic function at some point

@bgoodri @paul-buerkner Can you remind me, was the plan to release a posterior_epred() generic in rstantools before changing to it in rstanarm and brms or to transition to using the generic later? If the former, then should we go ahead and release an rstantools shortly?

@jgabry
Copy link
Member Author

jgabry commented May 14, 2020

I've added the family argument (for future use) as well as the version number in the returned list

@jgabry
Copy link
Member Author

jgabry commented May 14, 2020

Removed posterior_epred generic, importing it from rstantools now. bumped rstantools in DESCRIPTION to >= 2.0.0.9000 (latest version on GitHub that includes posterior_epred generic).

@jgabry
Copy link
Member Author

jgabry commented Jun 26, 2020

@bgoodri I just pushed some more stuff and I think that's pretty much everything I have. RAOS is supposedly being released in less than a month (July 23rd) so we need to have this branch on CRAN by then at the latest. We’ll need both rstan and rstantools on CRAN before then.

@jgabry jgabry changed the title [WIP] Updates to default priors Updates for Regression and Other Stories Jun 26, 2020
@jgabry jgabry requested a review from avehtari June 26, 2020 21:47
[ci skip]
Copy link
Contributor

@avehtari avehtari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the changes and provided some comments. I tested that code runs.

#' @export
#'
#' @templateVar stanregArg object
#' @template args-stanreg-object
#' @param transform Should the linear predictor be transformed using the
#' inverse-link function? The default is \code{FALSE}, in which case the
#' untransformed linear predictor is returned.
#' untransformed linear predictor is returned. The non-default behavior
#' of \code{TRUE} is somewhat deprecated since \code{posterior_epred}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "somewhat deprecated" mean? Is it possible to be more specific?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. @bgoodri by "somewhat deprecated" did you just mean that we don't recommend it anymore but we're not going to throw a warning (I think it's just a message currently not a warning)? Is there a reason not to deprecate it with a proper warning?

R/posterior_linpred.R Outdated Show resolved Hide resolved
@@ -102,6 +112,11 @@ posterior_linpred.stanreg <-
} else {
colnames(eta) <- rownames(newdata)
}

if (isTRUE(transform)) {
message("transform = TRUE is somewhat deprecated. ",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like use of "somewhat"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah "somewhat" is a bit strange. I think the idea was to distinguish between a message and warning, but I'm not sure. @bgoodri Are you ok if we change this message to a warning and get rid of "somewhat"?

vignettes/priors.Rmd Outdated Show resolved Hide resolved
@bgoodri
Copy link
Contributor

bgoodri commented Jun 30, 2020 via email

@jgabry
Copy link
Member Author

jgabry commented Jun 30, 2020

How about we say that it is "deprecated" instead of "somewhat deprecated" but we keep it a message instead of a warning?

@bgoodri
Copy link
Contributor

bgoodri commented Jun 30, 2020 via email

@jgabry
Copy link
Member Author

jgabry commented Jun 30, 2020

Ok in that case why don't we just say in the message that we recommend using posterior_epred instead (we don't need to say "deprecated" or "somewhat deprecated" at all). How about this?

message(
  "Instead of posterior_linpred(..., transform=TRUE) please call posterior_epred(), ",
  "which provides equivalent functionality."
)

@bgoodri
Copy link
Contributor

bgoodri commented Jun 30, 2020 via email

@jgabry
Copy link
Member Author

jgabry commented Jul 7, 2020

@bgoodri Ready to merge this?

@bgoodri
Copy link
Contributor

bgoodri commented Jul 8, 2020 via email

@bgoodri bgoodri merged commit 089fe78 into master Jul 8, 2020
@bgoodri bgoodri deleted the new-default-prior branch July 8, 2020 14:13
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.

None yet

3 participants