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

Included estimate of probability within ROPE #1570

Merged
merged 7 commits into from
Feb 22, 2021

Conversation

Rishabh261998
Copy link
Contributor

@Rishabh261998 Rishabh261998 commented Feb 17, 2021

Description

Included the estimate of probability within ROPE. To fix #1567

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@Rishabh261998
Copy link
Contributor Author

@OriolAbril could you take a look and give your feedback. Thanks

@OriolAbril
Copy link
Member

Will take a look whenever posssible, before that though, as the changes affect plotting, can you add some screenshots of the resulting plots?

@Rishabh261998
Copy link
Contributor Author

Rishabh261998 commented Feb 17, 2021

@OriolAbril
For: az.plot_posterior(data, var_names=['mu'], ref_val=0, rope=(-2, 3))

mpl

For: az.plot_posterior(data, var_names=['mu'], ref_val=0, rope=(-2, 3), backend="bokeh")

bokeh

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 :), I added a couple of cosmetic changes

Comment on lines 190 to 192
less_than_left_probability = (values < vals[0]).mean()
less_than_right_probability = (values <= vals[1]).mean()
probability_within_rope = less_than_right_probability - less_than_left_probability
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
less_than_left_probability = (values < vals[0]).mean()
less_than_right_probability = (values <= vals[1]).mean()
probability_within_rope = less_than_right_probability - less_than_left_probability
probability_within_rope = ((values > vals[0]) & (values <= vals[1])).mean()

I think this will perform better in terms of memory usage, values are not generally too large arrays but I think it's better to be cautious. It should return the same value.

probability_within_rope = less_than_right_probability - less_than_left_probability
ax.text(
x=(vals[0]+vals[1])/2,
y=[max_data * 0.5],
Copy link
Member

Choose a reason for hiding this comment

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

Can you try other values and see if you can get the rope text to be between the ref_val and hdi text?

@Rishabh261998
Copy link
Contributor Author

@OriolAbril is this ok?
For Matplotlib:
mpl
For Bokeh:
bokeh

@OriolAbril
Copy link
Member

Looks great now, thanks!

@Rishabh261998
Copy link
Contributor Author

Rishabh261998 commented Feb 18, 2021

Then I will commit the changes and also add it to the CHANGELOG.md
Was also thinking if the ROPE endpoints(also the %) could be made red in bokeh as it is quite confusing with the HDI endpoints, your views @OriolAbril?

@OriolAbril
Copy link
Member

If you are up for it I think ideally both plots would look the same, green for rope and orange for ref val. If you decide to work on this make sure to fetch the lastest changes to main branch and rebase

@Rishabh261998
Copy link
Contributor Author

Yes, I will work on this. Thanks @OriolAbril
btw why are these checks failing?

@OriolAbril
Copy link
Member

OriolAbril commented Feb 18, 2021

btw why are these checks failing?

It's the code style check, in this case black; here is a direct link to the message https://dev.azure.com/ArviZ/ArviZ/_build/results?buildId=3989&view=logs&j=af9a1d3a-ff8f-5483-6823-86518c773a9b&t=6a1cfc46-f4a4-5e3c-e2fb-18ea29f733bf&l=15, you can also get there by clicking details on the job and then the link to Azure.

@Rishabh261998
Copy link
Contributor Author

@OriolAbril does this work?
bokeh

@OriolAbril
Copy link
Member

OriolAbril commented Feb 18, 2021

Looks good, but it would be great if it was possible to hardcode the colors (in matplotlib the colors used are C0, C1... which are defined by the current style, you can try out different styles and see how the colors change).

Two options to do this come to mind, the first is to add rope_kwargs, ref_val_kwargs... which is what most plots do (and what I though plot_posterior did :/ ), however, I am not sure how many kwargs should be added, do we need different kwargs for rope text and for rope line?

The 2nd is to use our helper function vectorized_to_hex in the bokeh plot. like so:

     ax.line([val, val], [0, 0.8 * max_data], line_color=vectorized_to_hex("C1"), line_alpha=0.65) 

cc @ahartikainen @canyon289

@Rishabh261998
Copy link
Contributor Author

Rishabh261998 commented Feb 18, 2021

@OriolAbril I am familiar with the 2nd method, I have seen an issue regarding that
For the first do you mean to send the colours as arguments to both matplotlib and bokeh rope and ref_val functions so that the colour used is exactly the same in both the backend?
btw I was also unable to make the ROPE text bold in bokeh as it is in the case of matplotlib

@OriolAbril
Copy link
Member

For the first do you mean to send the colours as arguments to both matplotlib and bokeh rope and ref_val functions so that the colour used is exactly the same in both the backend?

The general approach in plotting functions is to have the base function (in arviz/plots) do some data wrangling and (sometimes also define some defaults as kwarg dictionaries) and then pass these dictionaries to the selected backend function which is the one that does the plotting. See in pairplot for example:

https://github.com/arviz-devs/arviz/blob/main/arviz/plots/backends/bokeh/pairplot.py#L123-L136

the defaults are taken from this matplotlib defaults in order to work with mpl styles out of the box, but it is also possible to use kwarg dicts for customization that goes further or diverges from the style.

The 2 approaches I described above are not exclusive, vectorized_to_hex would also be used in the kwargs option, the difference is that the color would not be so hardcoded. I don't remember seeing anyone complaining about the colors in plot_posterior, so maybe the 2nd option is enough to handle this as it already allows some level of customization

@Rishabh261998
Copy link
Contributor Author

Rishabh261998 commented Feb 20, 2021

@OriolAbril so I am facing this problem:

In the case of bokeh plot we have different arguments for rope_text and rope_line as you had asked above. The color argument also has different names i.e text_color and line_color respectively. So I was thinking that if we could do sth like this, we take a common kwargs_rope from the user which has arguments like text_color, line_color, line_width etc. Then in our display_rope function in bokeh we can define separate kwargs for text and line i.e kwargs_rope_ text and kwargs_rope_line and then do sth like this kwargs_rope_text.setdefault('text_color', kwargs_rope.get('text_color', default value)) similarly for other kwargs. So is this method ok?

Similarly for the case of matplotlib in that case though we have a common argument for color for line and text.
So if we follow this method then we also need to update the documentation and add the 'kwargs_rope'.

Similarly for kwargs_ref_val

@OriolAbril
Copy link
Member

Given that nobody has ever asked to customize the rope or ref val, I think allowing to change the color only should be enough. I am not sure the feature is worth all the effort to add kwargs or split them.

Having to use styles to change this seems fine to me even if it is a bit cryptic, maybe we could also add a rope/ref_val_color arg but more than that is probably too muchwwork for too little to be gained

@Rishabh261998
Copy link
Contributor Author

So I will just modify the code to incorporate only the color arguments(for both ref and rope)

@Rishabh261998
Copy link
Contributor Author

@OriolAbril could you look at the changes. Thanks

arviz/plots/backends/matplotlib/posteriorplot.py Outdated Show resolved Hide resolved
arviz/plots/posteriorplot.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #1570 (d487936) into main (cc89ade) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1570   +/-   ##
=======================================
  Coverage   89.99%   89.99%           
=======================================
  Files         107      107           
  Lines       11532    11535    +3     
=======================================
+ Hits        10378    10381    +3     
  Misses       1154     1154           
Impacted Files Coverage Δ
arviz/plots/posteriorplot.py 91.17% <ø> (ø)
arviz/plots/backends/bokeh/posteriorplot.py 98.19% <100.00%> (+0.03%) ⬆️
arviz/plots/backends/matplotlib/posteriorplot.py 98.27% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc89ade...b4e8fb7. Read the comment docs.

@Rishabh261998
Copy link
Contributor Author

@OriolAbril made the changes, can you review? Thanks

@OriolAbril OriolAbril merged commit a79695d into arviz-devs:main Feb 22, 2021
@OriolAbril
Copy link
Member

Thanks @Rishabh261998!

utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* Included estimate of probability within ROPE

* Positioned ROPE text and added to CHANGELOG

* Changed color scheme in plot_posterior in bokeh

* Changed size of rope text in posteriorplot bokeh

* Added rope_color and ref_val_color arguments to plot_posterior

* Minor Changes

* Some minor changes
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.

Include estimate of probability within RoPE when plotting
2 participants