Bullet Chart FF #872

Kully · 2017-11-07T19:27:50Z

NEEDED: Tests

update: tests added

Kully · 2017-11-07T20:31:38Z

Screenshots w/ code examples:

import plotly
import plotly.plotly as py
import plotly.figure_factory as ff
import plotly.colors as colors

import pandas as pd

fig = ff.create_bullet(
    df, as_rows=True
)
py.iplot(fig)

With Colorhunt theme:

import plotly
import plotly.plotly as py
import plotly.figure_factory as ff
import plotly.colors as colors

import pandas as pd

fig = ff.create_bullet(
    df, as_rows=False, marker_size=30, marker_symbol='400',measure_colors=['#476268', '#40A798'],
    range_colors=['#F5E1DA', '#F1F1F1']
)
py.iplot(fig)

Kully · 2017-11-08T17:14:31Z

@chriddyp ready for a review

chriddyp · 2017-11-08T17:23:31Z

It looks like we're using shapes to create these bars. Have you looked into using bar chart traces with width, offset, and base instead? Here's the PR that added it plotly/plotly.js#1075 and here is an example that I was making a couple of weeks ago for a separate project: https://codepen.io/chriddyp/pen/mBQVEy

chriddyp · 2017-11-08T17:24:54Z

The advantage of using bar charts with width and offset are:

Easier to load and style inside chart studio (https://plot.ly/create)
Easier for user to update their figure programatically
Hover labels

Kully · 2017-11-10T21:46:09Z

@chriddyp ready for a review now. The code has been reworked for bar traces 📊

chriddyp · 2017-11-10T22:33:08Z

plotly/colors.py

+
+    if colortype == 'rgb':
+        # covert back to rgb
+        inter_med_rgb = label_rgb(inter_med_tuple)


by rgb do you mean like a string like rgb(30, 20, 10)? if so, could we say convert back to an rgb string, e.g. rgb(30, 20, 10)

Yes that's what I mean. More clearer your way.

chriddyp · 2017-11-10T22:33:38Z

plotly/colors.py

    """
    Splits a low and high color into a list of n_colors colors in it

    Accepts two color tuples and returns a list of n_colors colors
    which form the intermediate colors between lowcolor and highcolor
-    from linearly interpolating through RGB space
+    from linearly interpolating through RGB space. If colortype is 'rgb'
+    the function will return a list of colors in the same form.


is this a typo? i thought tuple returned a list (well, a tuple) of colors?

I can see how there's confusion.

If colortype='tuple', lowcolor and highcolor must be what I have called tuple(-like) colors i.e. tuples of the form (a,b,c) where a, b, c are between 0 and 1, where (0,0,0) == rgb(0,0,0), etc.

If the input islowcolor=(0,0,0), highcolor=(1,1,1), n_colors=3 then the output is:

[(0, 0, 0), (0.5, 0.5, 0.5), (1, 1, 1)]

If colortype='rgb', then the list above will contain the rgb equivalent colors in a list

Also this function will error if you input tuple-colors and colortype is set to rgb. Another thing to fix.

chriddyp · 2017-11-10T22:34:06Z

plotly/colors.py

@@ -520,6 +542,10 @@ def n_colors(lowcolor, highcolor, n_colors):
                     lowcolor[2] + (index * incr_2))
        color_tuples.append(new_tuple)

+    if colortype == 'rgb':
+        # convert back to rgb
+        color_tuples = color_parser(color_tuples, label_rgb)


should this say color_rgb rather than color_tuples?

It should, but I kept it like that so that color_tuples is a common variable in both paths the function can take. Shall I change to list_of_colors for an apt name?

chriddyp · 2017-11-15T18:09:37Z

plotly/figure_factory/_bullet.py

+        DataFrame. All keys must be one of 'title', 'subtitle', 'ranges',
+        'measures', and 'markers'.
+    :param (bool) as_rows: if True, the bars are placed horizontally as rows.
+        If False, the bars are placed vertically in the chart.


Would orientation='v' | 'h' be more consistent with the rest of plotly here?

Yes again. 👍

chriddyp · 2017-11-15T18:11:04Z

plotly/figure_factory/_bullet.py

+
+    :param (pd.DataFrame | list) df: either a JSON list of dicts or a pandas
+        DataFrame. All keys must be one of 'title', 'subtitle', 'ranges',
+        'measures', and 'markers'.


This seems unlike the rest of our figure factories, but I might be mistaken. It seems odd to require users to reformat their dataframes. Would it be better if it was something like:

create_bullet(df, title='column a', measures='column b', markers='column c')

?

In other words, let users pass in whatever dataframe they want but then have them specify the column mapping

Haha, if there are they are probably ones I wrote. This makes way more sense, great suggestion.

chriddyp · 2017-11-15T18:24:18Z

plotly/figure_factory/_bullet.py

+            try:
+                r_is_nan = math.isnan(r)
+                if r_is_nan or r is None:
+                    df[needed_key][idx] = []


similarly here. if you need to change data, make a copy of their data and then mutate the copy

chriddyp · 2017-11-15T18:24:56Z

plotly/figure_factory/_bullet.py

+    fig['layout'].update(
+        title=title,
+        height=height,
+        width=width,


if this was layout_options then you could just pass in fig['layout'].update(layout_options)

chriddyp · 2017-11-15T18:25:57Z

plotly/tests/test_optional/test_figure_factory.py

+        )
+
+    def test_full_bullet(self):
+        df = pd.read_json('https://cdn.rawgit.com/plotly/datasets/master/BulletData.json')


Can we include this data in the repo itself? That way these tests don't break if that dataset is removed or if rawgit goes down.

chriddyp · 2017-11-15T18:26:55Z

plotly/figure_factory/_bullet.py

+    fig = plotly.tools.make_subplots(
+        num_of_rows, num_of_cols, print_grid=False,
+        horizontal_spacing=subplot_spacing,
+        vertical_spacing=subplot_spacing


does it make sense to provide horizontal_spacing and vertical_spacing as input arguments to ff.make_bullet instead? that way it's consistent with make_subplots

Yes it does!

chriddyp · 2017-11-15T18:28:53Z

plotly/figure_factory/_bullet.py

+    :param (str) title: title of the bullet chart.
+    :param (float) height: height of the chart.
+    :param (float) width width of the chart.
+    """


Could we include a couple of simple examples in here?

Kully · 2017-11-22T18:02:27Z

@chriddyp bumped ver num and CHANGELOG update

ready for a final 💃

chriddyp · 2017-11-22T21:01:45Z

plotly/figure_factory/_bullet.py

+
+        for k in scatter_options:
+            if k != 'marker':
+                markers[k] = scatter_options[k]


Could this be written just like:

go.Scatter( x=x, y=y, name='markers', hoverinfo='...', xaxis='...', yaxis='...', **scatter_options )

?

chriddyp · 2017-11-22T21:03:04Z

plotly/figure_factory/_bullet.py

+        of each subplot chart.
+    :param (bool) orientation: if 'h', the bars are placed horizontally as
+        rows. If 'v' the bars are placed vertically in the chart.
+    :param (int) marker_size: sets the size of the markers in the chart.


this is outdated

chriddyp · 2017-11-22T21:03:09Z

plotly/figure_factory/_bullet.py

+        rows. If 'v' the bars are placed vertically in the chart.
+    :param (int) marker_size: sets the size of the markers in the chart.
+    :param (str | int) marker_symbol: the symbol of the markers in the chart.
+        Default='diamond-tall'


chriddyp · 2017-11-22T22:13:48Z

plotly/figure_factory/_bullet.py

+        the rectangles for the range are drawn. These rectangles are meant to
+        be qualitative indicators against which the marker and measure bars
+        are compared.
+        Default=['rgb(198, 198, 198)', 'rgb(248, 248, 248)']


I don't see rgb(248, 248, 248) anywhere else in the code.

Would it make sense to supply this value directly in the function instead of None? i.e.

def create_bullet(data, markers=None, measures=None, ranges=None, subtitles=None, titles=None, orientation='h', range_colors=('rgb(198, 198, 198)', 'rgb(248, 248, 248)',), measure_colors=('rgb(31, 119, 280)', 'rgb(176, 196, 221)', ), horizontal_spacing=None, vertical_spacing=None, scatter_options={}, **layout_options):

(note that I'm using a tuple instead of a list because mutable default arguments aren't safe: http://docs.python-guide.org/en/latest/writing/gotchas/)

chriddyp · 2017-11-22T22:20:44Z

plotly/figure_factory/_bullet.py

+    # validate df
+    if not pd:
+        raise exceptions.ImportError(
+            "'pandas' must be imported for this figure_factory."


"imported" - it's really installed, not imported, right? The user doesn't need to do a import pandas before they call this function do they?
If so, could we correct this typo in any other place that it might appear

that's correct, it just needs to be installed on their system

chriddyp · 2017-11-22T22:22:41Z

plotly/figure_factory/_bullet.py

+    if isinstance(data, list):
+        if not all(isinstance(item, dict) for item in data):
+            raise exceptions.PlotlyError(
+                'If your data is a list, all entries must be dictionaries.'


I think it's confusing to lead with a "If your data is a list" - the user might wonder if there data is indeed a list or not. What about something like "Every entry of the data argument (a list) must be a dictionary."

good point!

How about this:

'Every entry of the data argument (a list, tuple, etc) must be a dictionary.'

👍 very nice

chriddyp · 2017-11-22T22:25:27Z

plotly/figure_factory/_bullet.py

+                [d[measures] for d in data] if measures else [[]] * len(data),
+                [d[ranges] for d in data] if ranges else [[]] * len(data),
+            ],
+            index=col_names


Rather than looping through every element and reconstructing a list, I believe that you can just pass in your list of dictionaries directly into pandas:

In [2]: pd.DataFrame([{'x': 1, 'y': 3}, {'x': 3, 'y': 10}]) Out[2]: x y 0 1 3 1 3 10

That's true, but I want to insert empty columns where a column name is not inserted.

Ex. if the user doesn't assign the titles variable to anything, I want a column named titles with empty strings in each cell of that column

Can I leave this the same way it is? I think it will be more straightforward than doing pd.DataFrame(data) then adding in the empty columns afterwards.

chriddyp · 2017-11-22T22:27:39Z

plotly/figure_factory/_bullet.py

+    ]
+
+    fig = ff.create_bullet(
+        data, titles='e', subtitles='d', markers='a', measures='b',


could we use more descriptive names than e, d, a, b?

yes can do. the point was to have titles that are different than the names of the columns that the second dataframe I construct in the figure factory.

chriddyp · 2017-11-22T22:29:25Z

💃 once my comments are addressed

Kully · 2017-11-23T00:03:16Z

What do you think also about allowing tuples as input arguments for the data?

There's no reason why not to allow this.

I was thinking of altering the part of code that checks if isinstance(data, list) to something like:

hasattr(data, index) for both lists and tuples

chriddyp · 2017-11-23T20:20:07Z

What do you think also about allowing tuples as input arguments for the data?

To an extent, python is a duck-typed language so we don't always need to explicitly check the type as long as the object that is supplied implements the right methods. So lists, tuples, numpy arrays, (and others) might look and feel the same and should all be accepted.

Here is a solution that checks if the object is a Sequence which is the base class for things like lists and tuples: https://stackoverflow.com/a/37842328/4142536

Kully added 8 commits November 6, 2017 12:23

added annotation dict func from facet_grid to utils

ff36387

add bullet ff

5b9ddaa

fix text_color name

939a8ba

added list_options to utils

5796a9e

working except variable measures

bb15f6a

enhance intermediate color function in colors.py

8a5ea2b

working ff

4599bc2

optional tests

beb6d18

added full test for bullet params

8ba62a2

reworked bullet charts with bar traces

69bd30d

chriddyp requested changes Nov 15, 2017

View reviewed changes

Kully added 7 commits November 16, 2017 11:14

not mutating data//pep8 in colors.py

69a541d

adding test file to figure_factory repo

e74996c

adding **kwargs - broken state

6da599f

finished chris comments and updated tests

79c67c7

remove json file and fixed test

9b9b578

ensure hor and ver spacing is always float

af9cb17

changelog and version number bump

6080d80

chriddyp reviewed Nov 22, 2017

View reviewed changes

few last comments

082fdfd

chriddyp reviewed Nov 22, 2017

View reviewed changes

chriddyp approved these changes Nov 22, 2017

View reviewed changes

Kully added 4 commits November 23, 2017 15:22

pandas imported message to 'installed'

dfd1b2c

rest of chris' comments

a2b0ca0

fix tests

ca9d37a

basestring to str

d0f5251

Kully merged commit 2ffbe1a into master Nov 23, 2017

Kully deleted the bullet_charts branch November 23, 2017 23:25

Bullet Chart FF #872

Bullet Chart FF #872

Conversation

Kully commented Nov 7, 2017 • edited Loading

Kully commented Nov 7, 2017 • edited Loading

Kully commented Nov 8, 2017

chriddyp commented Nov 8, 2017

chriddyp commented Nov 8, 2017

Kully commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kully commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriddyp commented Nov 22, 2017 • edited by Kully Loading

Kully commented Nov 23, 2017 • edited Loading

chriddyp commented Nov 23, 2017

Kully commented Nov 7, 2017 •

edited

Loading

Kully commented Nov 7, 2017 •

edited

Loading

Kully Nov 15, 2017 •

edited

Loading

chriddyp commented Nov 22, 2017 •

edited by Kully

Loading

Kully commented Nov 23, 2017 •

edited

Loading