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

Deprecate plotting functions in RStan or just replace backends? #517

Open
jgabry opened this issue Apr 18, 2018 · 12 comments
Open

Deprecate plotting functions in RStan or just replace backends? #517

jgabry opened this issue Apr 18, 2018 · 12 comments
Labels

Comments

@jgabry
Copy link
Member

jgabry commented Apr 18, 2018

Since we have bayesplot now there's no reason to also maintain all the old ggplot2 code currently in RStan. I guess we can

  • (A) deprecate the existing plotting functions in RStan with a message pointing to the bayesplot

or we can

  • (B) have the current functions in RStan call the bayesplot equivalent directly

Personally I'm in favor of (A) since it simplifies RStan's code base (we'd keep the deprecated functions for quite some time of course, but wouldn't plan to maintain that code in the long term). I'd like to do one or the other soon. What do people think of (A), (B), or some other option I haven't thought of?

@sakrejda
Copy link
Contributor

Deprecate! We can always backtrack on that if someone decides to go to town and update rstan to use bayesplot.

@jgabry
Copy link
Member Author

jgabry commented Apr 19, 2018

The only reason I can currently think of for not deprecating is that one feature in particular, unconstraining parameters, needs to be done with a stanfit object rather than the inputs bayesplot accepts. I suppose getting around that would involve providing a user friendly way to get a matrix/array of posterior draws in the unconstrained space to pass to bayesplot? It’s super annoying to do that currently.

@sakrejda
Copy link
Contributor

@jgabry Is the current method documented somewhere (sorry, I mostly use CmdStan...)

@avehtari
Copy link
Contributor

I suppose getting around that would involve providing a user friendly way to get a matrix/array of posterior draws in the unconstrained space

That would be a helpful function anyway.

@jgabry
Copy link
Member Author

jgabry commented Apr 22, 2018

@sakrejda unconstrain_pars()is the relevant RStan function.

http://mc-stan.org/rstan/reference/stanfit-method-logprob.html

But it’s annoying to use.

Sent with GitHawk

@jgabry
Copy link
Member Author

jgabry commented Apr 22, 2018

Here are the two main problems with unconstrain_pars() in my opinion: (1) the user interface is confusing and cumbersome. (2) can’t be used after reloading a model fit in a previous R session. We can fix (1) more easily than (2) but if RStan knew of and could perform the transformations used by Stan then we could fix (2) also. That should be possible but I don’t think we can do it easily in rstan without changes to Stan itself first.

Sent with GitHawk

@jgabry
Copy link
Member Author

jgabry commented Apr 22, 2018

@paul-buerkner I think in addition to bayesplot brms used to use some of the RStan plotting functions. Is that still the case? If so would you be ok with deprecating those in brms too if we go the route of deprecating the plotting in RStan?

Sent with GitHawk

@sakrejda
Copy link
Contributor

@jgabry riiiiiight.. this can't really be pulled apart until a) rstan tags parameter names with transforms; and b) transforms become available, ideally by being pulled out from the var_context stuff in Stan itself. I think this is doable over a few months (so like end of summer :) )

@jgabry
Copy link
Member Author

jgabry commented Apr 25, 2018

@sakrejda yeah that’s exactly what we need!

@jgabry
Copy link
Member Author

jgabry commented May 16, 2019

I will work on this soon. Except for the var_contextstuff that @sarkeda mentioned, which would be great but not my area of expertise.

@jgabry
Copy link
Member Author

jgabry commented May 16, 2019

I’m thinking deprecate with a message pointing to the analogous bayesplot function to the one the user called in rstan. Perhaps I can even get the message to be the exact call they would need for bayesplot.

@jgabry
Copy link
Member Author

jgabry commented May 16, 2019

Any opposition to that?

@jgabry jgabry pinned this issue May 16, 2019
@jgabry jgabry unpinned this issue May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants