-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Added rope to forestplot.py in forestplot() #448
Conversation
Sounds good. Can you post example? |
@ahartikainen Sure, by the way it allowed me to spot an error in the ymax definition. Reproducible example :
|
I like the idea, but it seems to me that the user should be allowed to set a rope per variable. |
Without having reviewed the details of the code, I would vote for
consistency between the various plots
…On Tuesday, December 11, 2018, Osvaldo Martin ***@***.***> wrote:
I like the idea, but it seems to me that the user should be allowed to set
a rope per variable.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#448 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AG4S4YOVTOp48XuKAEgD4D-QFIFtEkkSks5u398ngaJpZM4ZNCYm>
.
|
Sure I get it, I have no use for it but some might and consistency is defintely important. I will adapt the rope code of posteriorplot() to forestplot() when I have a bit time and perhaps generalize it to the ridgeplot option. Thanks for this highly useful package by the way. |
Great! And thank you for this contribution. |
Initial commit arviz-devs#448 completely rewritten. Rope can be a list of 2 or a dict like in posteriorplot() passed to the rope argument Argument rope_values writes ROPE values in plot when multiple ropes are given.
However, I don't master classes so I might have introduced a lot of redundancy... |
Faster reproducible and more complete example taken from the gallery :
|
I like it. I think the rope should not be labeled here. I know it is for the posterior plot, by for that plot the hpd is also labeled. |
You're right, I was hesitating whether to keep it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running `./scripts/lint.sh/ I'm getting the following error
- SRC_DIR=/home/tbayes/repos/arviz
- echo 'Checking documentation...'
Checking documentation...- python -m pydocstyle --convention=numpy /home/tbayes/repos/arviz/arviz/
/home/tbayes/repos/arviz/arviz/plots/forestplot.py:246 in public methoddisplay_multiple_ropes
:
D102: Missing docstring in public method
@GWeindel Thank you for the quick edits. I'll do another round of review either today or tomorrow. |
@GWeindel Thanks for the fixes. Unfortunately our CI checks are broken at this commit so I'm running things manually as you make changes. Here's the errors I'm getting. If you'd like you can rebase your changes on top of the current arviz master to get CI checks to work again. If you need help with this let me know. Alternatively you can run
and it will generate the output below
|
LGTM. I fixed some annoying pylint errors. |
Does not LGTM yet, https://coveralls.io/builds/20848553/source?filename=arviz%2Fplots%2Fforestplot.py#L257 |
Give me a couple more days, got caught up with other tasks |
Trying to add testing but messing up on arguments somewhere. The following code generates an exception
will continue working on this |
This implementation seems to break when each variable has multiple chains Combined = True works
Combined = False fails
I have a couple suggested paths but happy to hear any others
I won't have time to code solution 1 if we choose to go that route, but can do solution 2. |
I won't have the time (and perhaps sufficient skills) for solution 1. Hence I would suggest providing a warning. |
So what fails is
Which would mean that we don't have label for all edit. Line 331
to
|
What happens is that ArviZ is trying to match the label with the line. When the chains are compressed label position matches the y position in the numpy array so one value is found, we get it out of the array in index zero. In multi chain the ticks, and the y position, are off every so slightly, .175 if I remember correctly. The equality operator doesn't match anything and the array is empty, hence the index error. I hope this not so great description yields some insight. I can provide more detail if needed |
Ok We need y_group variable which is the mean of y locations per parameter (or value to be plotted). VarHandler probably needs to calculate these before hand, because all the yielding is disabling to do that later? |
Initial commit arviz-devs#448 completely rewritten. Rope can be a list of 2 or a dict like in posteriorplot() passed to the rope argument Argument rope_values writes ROPE values in plot when multiple ropes are given.
Now we just need to write a couple more tests and we're good! |
The new argument rope adds a shaded region of practical equivalence for all displayed variables in forestplot, shade is controlled by rope_alpha (default =.5). Contrary to posteriorplot rope can only be a tuple of length 2. Not big but it is highly useful for me hence I assume it should be useful for others too.
taking len(values) instead of values[-1] for ymax argument in axvspan()
Initial commit arviz-devs#448 completely rewritten. Rope can be a list of 2 or a dict like in posteriorplot() passed to the rope argument Argument rope_values writes ROPE values in plot when multiple ropes are given.
Fixed bug in ticks and title when specifying mutliple ROPES
- Removed unused variables - Moved the label/ticks collection to the case where a dict > 2 is provided - Added doc string to display_multiple_ropes() - Removed unused Variable xt_labelsize from ridgeplot() arguments
Thanks for the contribution @GWeindel |
Great ! Thanks for the help with the multilple chains and the tests ! |
The new argument rope adds a shaded region of practical equivalence for all displayed variables in forestplot, shade is controlled by rope_alpha (default =.5). Contrary to posteriorplot rope can only be a tuple of length 2.
Not big but it is highly useful for me hence I assume it should be useful for others too.