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

stanreg, brmsfit tidiers are inconsistent with tidyMCMC #291

Open
jacob-long opened this Issue Mar 16, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@jacob-long

jacob-long commented Mar 16, 2018

I see that @paul-buerkner wrote the method for brmsfit objects and I can anticipate some of the reasons he may have done this intentionally, but I'm not sure whether the current format makes sense.

While some aspects (par_type, parameters, etc.) are related to broader issues with mixed models (e.g., #96), that's not what I'm worried about since there are ongoing discussions about that. Instead, I'm concerned about the "confidence" intervals.

tidyMCMC is basically consistent with the general templates: we have conf.int and conf.level arguments and it returns columns with names conf.low and conf.high.

tidy.brmsfit and tidy.stanreg, on the other hand, use intervals and prob arguments and return corresponding columns called lower and upper.

I would say that at minimum, the inconsistency between tidyMCMC and tidy.brmsfit/tidy.stanreg doesn't make sense. Now I suspect tidy.brmsfit/tidy.stanreg opted not use any reference to "conf" because the intervals are not really confidence intervals. My opinion is that it would be better to stick with the broom norms (using the "conf" prefix) since users should understand the distinction and it is better than breaking any function that relies on the generality of tidy (like the one in my package that led me to realize this issue). I assume there are other model types supported by broom using "conf" that produce intervals that are also not truly confidence intervals, but I'm not certain.

But if others disagree with me about the nomenclature, I think it would then be best to change tidyMCMC so that someone who is willing to deal with MCMC models in this specific way doesn't have to switch back and forth between naming schemes to support both brmsfit/stanreg and stanfit/rjags/whatever else objects.

@alexpghayes

This comment has been minimized.

Show comment
Hide comment
@alexpghayes

alexpghayes Jun 4, 2018

Collaborator

I would love to use lower and upper for intervals in general, but we're probably too locked into conf.low and conf.high at this point. I think we should probably use conf.low and conf.high for all intervals throughout broom where it isn't egregiously misleading.

Will come back to this later after there's been time for people to discuss.

Collaborator

alexpghayes commented Jun 4, 2018

I would love to use lower and upper for intervals in general, but we're probably too locked into conf.low and conf.high at this point. I think we should probably use conf.low and conf.high for all intervals throughout broom where it isn't egregiously misleading.

Will come back to this later after there's been time for people to discuss.

@alexpghayes

This comment has been minimized.

Show comment
Hide comment
@alexpghayes

alexpghayes Jun 13, 2018

Collaborator

This discussion should also happen in broom.mixed since most of the Bayesian tidiers are migrating over there. Linking to bbolker/broom.mixed#2.

Collaborator

alexpghayes commented Jun 13, 2018

This discussion should also happen in broom.mixed since most of the Bayesian tidiers are migrating over there. Linking to bbolker/broom.mixed#2.

@bbolker

This comment has been minimized.

Show comment
Hide comment
@bbolker

bbolker Aug 21, 2018

Contributor

I think this can be closed. tidy.brmsfit and tidy.stanreg are now more broom-compliant. I did discuss this with @paul-buerkner and he seemed OK with it; I haven't spoken to the original stanreg authors, I hope they're not too unhappy. In any case, if this is still/becomes an issue, it should be re-opened in the broom.mixed issues list.

Contributor

bbolker commented Aug 21, 2018

I think this can be closed. tidy.brmsfit and tidy.stanreg are now more broom-compliant. I did discuss this with @paul-buerkner and he seemed OK with it; I haven't spoken to the original stanreg authors, I hope they're not too unhappy. In any case, if this is still/becomes an issue, it should be re-opened in the broom.mixed issues list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment