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

Fix ess/rhat plot in plot_forest #1606

Merged
merged 4 commits into from
Mar 19, 2021

Conversation

Rishabh261998
Copy link
Contributor

@Rishabh261998 Rishabh261998 commented Mar 8, 2021

Description

To fix #1596
As it was seen in #1591 that for the NoModel labeller the ess/rhat dots were plotted for only one model for both matplotlib and bokeh.

After modifying the code:
Matplotlib with NoModel labeller:
Screenshot from 2021-03-09 01-39-45

Bokeh:
Screenshot from 2021-03-09 01-39-17

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@ahartikainen
Copy link
Contributor

Are the bokeh y-limits a bit off? We could share them?

@Rishabh261998
Copy link
Contributor Author

Yeah, it looks like they are a bit off. I will look into this. Thank You

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #1606 (d18da77) into main (8d3a615) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d18da77 differs from pull request most recent head 6e6c8b1. Consider uploading reports for the commit 6e6c8b1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1606      +/-   ##
==========================================
- Coverage   90.92%   90.92%   -0.01%     
==========================================
  Files         108      108              
  Lines       11645    11663      +18     
==========================================
+ Hits        10588    10604      +16     
- Misses       1057     1059       +2     
Impacted Files Coverage Δ
arviz/plots/backends/bokeh/forestplot.py 94.91% <100.00%> (-0.16%) ⬇️
arviz/plots/backends/matplotlib/forestplot.py 97.85% <100.00%> (-0.26%) ⬇️
arviz/data/io_numpyro.py 94.48% <0.00%> (ø)
arviz/data/inference_data.py 83.59% <0.00%> (ø)

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 ef4f13c...6e6c8b1. Read the comment docs.

@Rishabh261998
Copy link
Contributor Author

Rishabh261998 commented Mar 9, 2021

@ahartikainen done!, Is there anything else which I need to change? Thanks
Screenshot from 2021-03-10 02-33-41

@Rishabh261998
Copy link
Contributor Author

@OriolAbril is there anything else that needs to be done here? Thanks

Comment on lines 200 to 206
for i in range(idx):
axes[0, i].y_range._property_values[
"start"
] = -all_plotters[ # pylint: disable=protected-access
0
].group_offset
axes[0, i].y_range._property_values["end"] = y_max # pylint: disable=protected-access
Copy link
Member

Choose a reason for hiding this comment

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

can this go above? I think the ymax definition does not depend on anything in between and can also be moved before the loop above. There already is a loop over axes, now that we loop here for the yaxis lims I think we can use the same loop that starts in line 175, mostly for clarity

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

LGTM

@OriolAbril OriolAbril merged commit 92f2f7a into arviz-devs:main Mar 19, 2021
@OriolAbril
Copy link
Member

Thanks @Rishabh261998, sorry it took a while to get it merged

utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* Fix ess/rhat plot in plot_forest

* Aligned ess/rhat dots in forestplot bokeh

* Minor changes

Co-authored-by: Oriol Abril-Pla <[email protected]>
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.

Fix ess/rhat plotting in plot_forest
3 participants