-
-
Notifications
You must be signed in to change notification settings - Fork 87
adding discrete option to ppc_rootogram #362
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #362 +/- ##
==========================================
- Coverage 98.60% 98.24% -0.37%
==========================================
Files 35 35
Lines 5661 5689 +28
==========================================
+ Hits 5582 5589 +7
- Misses 79 100 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks @behramulukir. I like this new style. One question for now: should we give users control over the color of the observed points since these colors may not be optimal for all the available color schemes? In most of the other plots the colors for everything come from the color scheme, but I see that it would be difficult to do that here if we want to really distinguish between observations that fall inside and outside the interval. We don't usually have arguments that control the colors, but I guess we could do that here. Another option would be to use the shape of the point (instead of the color) to distinguish whether it falls inside and outside the interval and then use a color that comes from the chosen color scheme. |
Yes @jgabry, I think it would be better to offer more colour options, which would be more inclusive. The current colour choices may be indistinguishable for some types of colour blindness. The only issue here is not confusing users about that colour control arguments can only be used with Distinguishing by the shape of the point also makes sense, but then we need to include both the inside interval and outside interval shapes in the legend to make things clear. I'll think about which implementation is less confusing for the users and try out different implementations. |
Based on our discussions with @TeemuSailynoja, we decided that it is a better choice to indicate observations within and out of the bounds by shapes of the points instead of colours, because as you said those colors should be available for each color scheme, however, adding a dedicated color argument that would only work if the However, there are nuances to these changes which we need to decide on before merging this PR to main. ShapesAs we decided to indicate whether observed points fall into intervals or not, the first shape option that we picked was triangles. So, we used downward triangles for observation points that fall into intervals and upward triangles for observation points that fall outside of the intervals. Here is an example of it: ![]() We also had the idea of using squares to indicate that observed points are within the intervals and rhombuses to indicate they are not. Here is an example of it: ![]() We didn't settle on any of these options, so please tell me which one you think is better looking. ColorsAnother point of building this new style of rootograms was deciding on how to colour different elements. Since we decided not to use colour to indicate the position of observed points, all of the visual elements should follow from the colour scheme that users pick. To do that, there were two main paths: following from other rootogram styles (standing, hanging, suspended), where observed points are light and expected points are dark or following from PPC intervals, where observed points are dark and expected points are light. The argument for following other rootogram styles, where observed points are light and expected points are dark, is that we have the same colouring practice for all kinds of rootograms. On the other hand, it is worth noting that Here is the example for lighter observed - darker expected: ![]() Here is the example for darker observed - lighter expected: ![]() One thing we also decided to change was removing the change in alpha of the lines where until the halfway, line had higher alpha and from halfway to the end of line it was lower alpha. This wasn't contributing much to user's understanding of the situation and adding another visual effect to a graph that already has lots of different visual cues. Removing this and having uniform alpha for the full lines made the graph easier to understand. |
Thanks for the detailed summary. Here are a few comments:
|
Thanks for your comments, @jgabry! I have a slight preference towards using squares and rhombuses instead of triangles, since the directions of triangles don't have a meaning on their own -nothing would change if we swap the directions- but it might signal users that there actually is a meaning, so if we use squares and rhombuses, we can avoid that. So, I'll go with them instead -unless anyone has an argument against it. For the colour, I agree with your comment. I am also in favour of copying |
I have a slight preference for circles (in), triangles (out). I think those are visually easier to differentiate. And having an option to enable/disable the distinction. |
To be honest, I am in favour of using circles only for expected points, as the colours we use for distinction are not completely different colours but shades of the same colour, which might be harder to distinguish for some people, and especially for people with visual impairments. I also think this can be a future update to I agree with your comment that the expected points should be on top of their lines. I checked |
In #360, I explained the rationale behind adding a new
discrete
option to thestyle
argument ofppc_rootogram()
to offer a new plot aesthetic which puts more emphasis on the discreteness of the data. This PR is about my work on that issue. The only difference between my implementation and what @TeemuSailynoja proposed is that I decided to use rhombuses instead of dots forObserved
values for better distinction.This implementation passes all existing tests, so I think the current style options for
ppc_rootogram()
remain unchanged.discrete
option tostyle
argumentExamples
Here are some examples of how
ppc_rootogram()
works withdiscrete
style.