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

Add transform argument to plots #1036

Merged
merged 17 commits into from
Feb 6, 2020
Merged

Conversation

amukh18
Copy link
Contributor

@amukh18 amukh18 commented Jan 29, 2020

Description

This pull request adds a much-needed `transform` argument to the function plot_trace. Fixes #1018

Checklist

  • Does the PR follow official
    PR format?
  • Has included a sample plot to visually illustrate the changes? (only for plot-related functions)
  • Is the new feature properly documented with an example?
  • Does the PR include new or updated tests to cover the new feature (using pytest fixture pattern)?
  • Is the code style correct (follows pylint and black guidelines)?
  • Is the change listed in changelog?

@amukh18
Copy link
Contributor Author

amukh18 commented Jan 29, 2020

@ahartikainen as suggested, I set the default of the transform argument to None. But I get the error
TypeError: 'NoneType' object is not callable

@@ -137,6 +140,9 @@ def plot_trace(
divergence_data = False

data = get_coords(convert_to_dataset(data, group="posterior"), coords)

data = transform(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

By doing this you are basically writing data = None(data), which is not legal Python. Instead you should do something like:

if transform is not None:
    data = transform(data)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aloctavodia Sorry for this elementary mistake! I fixed it.

@@ -46,6 +47,8 @@ def plot_trace(
Coordinates of var_names to be plotted. Passed to `Dataset.sel`
divergences : {"bottom", "top", None, False}
Plot location of divergences on the traceplots. Options are "bottom", "top", or False-y.
transform : callable
Function to transform data (defaults to identity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a better wording will be "Function to transform data (defaults to None i.e. the identity function)" or probably just "Function to transform data (defaults to None)"

@aloctavodia
Copy link
Contributor

aloctavodia commented Jan 29, 2020

@amukh18 Also notices that plot_trace is not the only function needing this new argument. Other functions are plot_forest, plot_pair, plot_posterior, plot_rank, plot_parallel, plot_violin, plot_density, plot_joint.

@aloctavodia
Copy link
Contributor

@amukh18 any update on this?

@amukh18
Copy link
Contributor Author

amukh18 commented Feb 4, 2020

@aloctavodia I deeply apologise for the long delay but I was caught up in my coursework and exams. I am relatively free now and have made the necessary commits.

@amukh18
Copy link
Contributor Author

amukh18 commented Feb 4, 2020

I have spotted some errors. I will recommit

@amukh18
Copy link
Contributor Author

amukh18 commented Feb 4, 2020

My code passed local pylint checks but the Travis CI build failed saying there was a pylint error

@aloctavodia aloctavodia changed the title [WIP] To add transform argument to plot_trace Add transform argument to plots Feb 5, 2020
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Before merging, could you add an example in plot_violin docstring (like it is done in the other plots) with two graphics? First a general violinplot without much customization and then showcase the usage of transform, something like az.plot_vioin(idata, var_names="tau", transform=np.log) You can do it in another PR if you prefer or open an issue and leave this for somebody else

@amukh18
Copy link
Contributor Author

amukh18 commented Feb 6, 2020

@OriolAbril No problem!
And yes, I'll add those examples

@amukh18
Copy link
Contributor Author

amukh18 commented Feb 6, 2020

@OriolAbril I have created the pull request and am working on it.

@OriolAbril
Copy link
Member

I am merging this so examples can be added on top of this using transform already

@OriolAbril OriolAbril merged commit 3899c39 into arviz-devs:master Feb 6, 2020
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.

Add transform argument
3 participants