-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
Conversation
* default to FALSE in functions for setting priors * explicitly set to TRUE in default prior usage to keep autoscaling by default
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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Comments from discussion:
|
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 |
I've added the |
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). |
[ci skip]
[ci skip]
don't use range(x) anymore
[ci skip]
@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. |
[ci skip]
There was a problem hiding this 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.
R/posterior_linpred.R
Outdated
#' @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} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -102,6 +112,11 @@ posterior_linpred.stanreg <- | |||
} else { | |||
colnames(eta) <- rownames(newdata) | |||
} | |||
|
|||
if (isTRUE(transform)) { | |||
message("transform = TRUE is somewhat deprecated. ", |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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"?
I don't know if we'll ever get rid of the option but posterior_epred is
preferable.
…On Mon, Jun 29, 2020, 1:20 PM Jonah Gabry ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In R/posterior_linpred.R
<#432 (comment)>:
> #' @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}
Good question. @bgoodri <https://github.com/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?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#432 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKUUGPQKJH4FE34O4HLRZDEMFANCNFSM4MSCIJ4A>
.
|
How about we say that it is "deprecated" instead of "somewhat deprecated" but we keep it a message instead of a warning? |
I think deprecated means the argument is going away, which I am not sure of
…On Tue, Jun 30, 2020, 11:44 AM Jonah Gabry ***@***.***> wrote:
How about we say that it is "deprecated" instead of "somewhat deprecated"
but we keep it a message instead of a warning?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#432 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKXEIQK2EDDKCW4OVYDRZIB5TANCNFSM4MSCIJ4A>
.
|
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."
) |
ok
…On Tue, Jun 30, 2020 at 7:06 PM Jonah Gabry ***@***.***> wrote:
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."
)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#432 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKTYI6PKYPCYY2BTCIDRZJVYJANCNFSM4MSCIJ4A>
.
|
@bgoodri Ready to merge this? |
sure
…On Tue, Jul 7, 2020 at 6:47 PM Jonah Gabry ***@***.***> wrote:
@bgoodri <https://github.com/bgoodri> Ready to merge this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#432 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKXOCX3L3HXZRHZW2VLR2OQZNANCNFSM4MSCIJ4A>
.
|
@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:
This means that informative priors don't require explicitly setting autoscale=FALSE, as requested by @andrewgelman and @avehtari for RAOS.