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

Outside_bottom legend breaks with Bokeh 1.2 #81

Closed
cphalpert opened this issue Jun 7, 2019 · 5 comments · Fixed by #87
Closed

Outside_bottom legend breaks with Bokeh 1.2 #81

cphalpert opened this issue Jun 7, 2019 · 5 comments · Fixed by #87
Labels
good first issue Good for newcomers

Comments

@cphalpert
Copy link
Contributor

Bokeh 1.2.0:

/usr/local/lib/python3.6/dist-packages/confidence/analysis/base.py in _summary_plot(self, level_name, level_df, remaining_groups, groupby)
    233 
    234             ch = self._ordinal_summary_plot(level_name, level_df,
--> 235                                             remaining_groups, groupby)
    236         else:
    237             ch = self._categorical_summary_plot(level_name, level_df,
/usr/local/lib/python3.6/dist-packages/confidence/analysis/base.py in _ordinal_summary_plot(self, level_name, level_df, remaining_groups, groupby)
    249         return self._ordinal_plot('point_estimate', df, groupby, level_name,
    250                                   remaining_groups, absolute=True,
--> 251                                   title=title, y_axis_label=y_axis_label)
    252 
    253     def _ordinal_plot(self, center_name, df, groupby, level_name,
/usr/local/lib/python3.6/dist-packages/confidence/analysis/base.py in _ordinal_plot(self, center_name, df, groupby, level_name, remaining_groups, absolute, title, y_axis_label)
    278         ch.set_title(title)
    279         if colors:
--> 280             ch.set_legend_location('outside_bottom')
    281         return ch
    282 
/usr/local/lib/python3.6/dist-packages/chartify/_core/chart.py in set_legend_location(self, location, orientation)
    332                 self._subtitle_glyph.text)
    333         elif location == 'outside_bottom':
--> 334             add_outside_legend('bottom_center', 'below')
    335         elif location == 'outside_right':
    336             add_outside_legend('top_left', 'right')
/usr/local/lib/python3.6/dist-packages/chartify/_core/chart.py in add_outside_legend(legend_location, layout_location)
    320                 return self
    321             new_legend = self.figure.legend[0]
--> 322             new_legend.plot = None
    323             new_legend.orientation = orientation
    324             self.figure.add_layout(new_legend, layout_location)
/usr/local/lib/python3.6/dist-packages/bokeh/core/has_props.py in __setattr__(self, name, value)
    286 
    287             raise AttributeError("unexpected attribute '%s' to %s, %s attributes are %s" %
--> 288                 (name, self.__class__.__name__, text, nice_join(matches)))
    289 
    290     def __str__(self):
AttributeError: unexpected attribute 'plot' to Legend, possible ```
@bryevdv
Copy link

bryevdv commented Jun 12, 2019

I don't think Chartify should have ever been setting new_legend.plot = None in the first place, I would suggest just removing that line.

@cphalpert
Copy link
Contributor Author

This change makes chartify incompatible with older versions of bokeh, e.g. 1.0.4. (didn't test extensively to find the exact version, though >=1.2 might suffice).

We should add a lower bound to the bokeh requirements

@cphalpert cphalpert reopened this Sep 17, 2019
@Aashishthakur10
Copy link

Aashishthakur10 commented Oct 12, 2019

@cphalpert Does this mean adding the new_legend.plot = None back but with some condition based on version <=1.2?

@bryevdv
Copy link

bryevdv commented Oct 12, 2019

I would personally advise against that, and suggest that as a downstream library you are free to be opinionated about what minimum versions you support. Just make sure to put a minimum Bokeh version in the next release and update as appropriate in future releases. You might also want to put an upper bound < next major version as well (Bokeh 2.0 is on the near horizon)

@cphalpert
Copy link
Contributor Author

Added a proper bokeh minimum version in #98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants