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

Missing kwarg in new traceplot #3420

Closed
junpenglao opened this issue Mar 21, 2019 · 10 comments
Closed

Missing kwarg in new traceplot #3420

junpenglao opened this issue Mar 21, 2019 · 10 comments

Comments

@junpenglao
Copy link
Member

junpenglao commented Mar 21, 2019

We used to have a kwarg transform that you can pass a element-wise function to do a transform on the mcmc sample:

pm.traceplot(short_trace, var_names=['tau'], transform=np.log);

Not sure if this is still a useful feature, if so we should add it to arviz, if not we should remove all the reference to it in our doc and modify those plotting accordingly.

@ColCarroll @canyon289 @aloctavodia

@junpenglao
Copy link
Member Author

Related, is there a way to plot the transformed parameters? For example in the trace there is tau_log__ but doing pm.traceplot(short_trace, var_names=['tau_log__']) returns null

@junpenglao
Copy link
Member Author

junpenglao commented Mar 21, 2019

We can change include_transformed=True in
https://github.com/arviz-devs/arviz/blob/master/arviz/data/io_pymc3.py#L60-L68

Or make include_transformed into a kwarg and add to all the plots (which is the old PyMC3 plotting behavior).

WDYT?

@twiecki
Copy link
Member

twiecki commented Mar 21, 2019

Hm, yeah that's tricky. I think it makes sense for arviz to not care about "hidden" variables, especially if that relies on a particular way to code them. As such, placing it inside of the pymc3 xarray converter makes sense. Then we could probably still keep the old behavior in our wrapper, no?

I never used the transform stuff, didn't know it existed. I think just manually transforming the samples should be good enough.

@aloctavodia
Copy link
Member

We removed the transform argument from ArviZ, based on the general design guide that if the user wants to transform something they should do it before passing the data to ArviZ.

Regarding transformed variables I remember we discuss that internally and also here arviz-devs/arviz#230, but that's all I remember :-)

@junpenglao
Copy link
Member Author

Hmm seems like this is a longer discussion to have, I will move forward of rerunning the doc for now and remove/modify the places where we are plotting with include_transformed=True.

@ColCarroll
Copy link
Member

One idea is that pm.traceplot still exists, and include_transformed can go in there (probably to just mess with the xarray object, then pass it forward into az.plot_trace)

@fonnesbeck
Copy link
Member

fonnesbeck commented Apr 12, 2019

I think its worth having post-hoc transformation available as an argument, because it more easily allows users to explore their output and iterate over different transformations.

@clausherther
Copy link

clausherther commented May 29, 2019

There may be some good philosophical reasons to remove transform, but to possibly point out the obvious, this change in a point release breaks a lot of code not so gracefully.
TypeError: plot_trace() got an unexpected keyword argument 'transform'

This has been my experience now with all the commonly used new plotting functions. Much of my plotting code needs some sort of recoding after the upgrade.

@rpgoldman
Copy link
Contributor

Maybe choose how to address this, push it to arviz, and close it here?

@OriolAbril
Copy link
Member

I think this was fixed a while back in arviz-devs/arviz#1036

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

No branches or pull requests

8 participants