Added parameter to choose color_threshold #1075
Conversation
There was a problem hiding this comment.
Hi @Elpiro , thanks a lot for this contribution.
Please take a look at the comments below, and see if you can fix the plotly.tests.test_optional.test_figure_factory.test_figure_factory.TestDendrogram test that failing on CircleCI.
plotly/figure_factory/_dendrogram.py
Outdated
| x=np.multiply(self.sign[self.xaxis], xs), | ||
| y=np.multiply(self.sign[self.yaxis], ys), | ||
| mode='lines', | ||
| marker=dict(color=colors[color_key]), | ||
| marker=graph_objs.Marker(color=colors[color_key]), |
There was a problem hiding this comment.
Stick with dict or use graph_objs.scatter.Marker. graph_objs.Marker is deprecated in version 3
plotly/figure_factory/_dendrogram.py
Outdated
|
|
||
|
|
||
| class _Dendrogram(object): | ||
| """Refer to FigureFactory.create_dendrogram() for docstring.""" | ||
|
|
||
| def __init__(self, X, orientation='bottom', labels=None, colorscale=None, | ||
| width=np.inf, height=np.inf, xaxis='xaxis', yaxis='yaxis', | ||
| width="100%", height="100%", xaxis='xaxis', yaxis='yaxis', |
There was a problem hiding this comment.
"100%" is not a valid height/width and it looks like this is causing your test failure.
Where you trying to change some behavior here? I'm not sure why the old value was inf. I think the default should probably be just None, which will let the rending approach (plot, iplot, or FigureWidget) decide how to size the figure.
plotly/figure_factory/_dendrogram.py
Outdated
| hovertext) | ||
| linkagefun, | ||
| hovertext, | ||
| color_threshold) |
There was a problem hiding this comment.
Why is there no closing parenthesis here?
plotly/figure_factory/_dendrogram.py
Outdated
|
|
||
|
|
||
| class _Dendrogram(object): | ||
| """Refer to FigureFactory.create_dendrogram() for docstring.""" | ||
|
|
||
| def __init__(self, X, orientation='bottom', labels=None, colorscale=None, | ||
| width=np.inf, height=np.inf, xaxis='xaxis', yaxis='yaxis', | ||
| width="100%", height="100%", xaxis='xaxis', yaxis='yaxis', |
There was a problem hiding this comment.
"100%" isn't a valid for a width/height. This is what's causing your test failure. I think this should probably be None, which will let the rendering approaches (plot, iplot, or FigureWidget) decide what size to use.
plotly/figure_factory/_dendrogram.py
Outdated
| @@ -30,6 +30,7 @@ def create_dendrogram(X, orientation="bottom", labels=None, | |||
| :param (function) linkagefun: Function to compute the linkage matrix from | |||
| the pairwise distances | |||
| :param (list[list]) hovertext: List of hovertext for constituent traces of dendrogram | |||
| :param (double) color_threshold: Value at which the separation of clusters will be made | |||
There was a problem hiding this comment.
Please align with :param above
plotly/figure_factory/_dendrogram.py
Outdated
| @@ -144,7 +145,7 @@ def __init__(self, X, orientation='bottom', labels=None, colorscale=None, | |||
| self.zero_vals.sort() | |||
|
|
|||
| self.layout = self.set_figure_layout(width, height) | |||
| self.data = dd_traces | |||
| self.data = graph_objs.Data(dd_traces) | |||
There was a problem hiding this comment.
graph_objs.Data is deprecated in version 3. Just stick with dd_traces as a list or tuple.
plotly/figure_factory/_dendrogram.py
Outdated
| @@ -262,7 +263,8 @@ def get_dendrogram_traces(self, X, colorscale, distfun, linkagefun, hovertext): | |||
| d = distfun(X) | |||
| Z = linkagefun(d) | |||
| P = sch.dendrogram(Z, orientation=self.orientation, | |||
| labels=self.labels, no_plot=True) | |||
| labels=self.labels, no_plot=True, | |||
| color_threshold = color_threshold) | |||
There was a problem hiding this comment.
please align with labels= above
plotly/figure_factory/_dendrogram.py
Outdated
| x=np.multiply(self.sign[self.xaxis], xs), | ||
| y=np.multiply(self.sign[self.yaxis], ys), | ||
| mode='lines', | ||
| marker=dict(color=colors[color_key]), | ||
| marker=graph_objs.Marker(color=colors[color_key]), |
There was a problem hiding this comment.
plotly/figure_factory/_dendrogram.py
Outdated
| @@ -288,12 +290,11 @@ def get_dendrogram_traces(self, X, colorscale, distfun, linkagefun, hovertext): | |||
| hovertext_label = None | |||
| if hovertext: | |||
| hovertext_label = hovertext[i] | |||
| trace = dict( | |||
| type='scatter', | |||
| trace = graph_objs.Scatter( | |||
There was a problem hiding this comment.
Let's stick with dict (See below)
|
Hi @Elpiro , Are you interested in continuing work on this? I'm hoping to release version 3.2 in a couple of weeks, so I think there's still time to get this merged before then. Otherwise we can just bump it to 3.3 (which is totally fine) Also, the integration test suite was migrated to CircleCI 2.0, so you should merge in master before pushing more changes. Thanks! |
|
I won't have time to finish this for 3.2, but it should be done for 3.3.
I find this weird that it raised so much changes during the git commit, and
that it raises problems on CircleCI, since I just changed 1 or 2 lines of
code. It's my first time editing a github project so I'm still getting used
to these tools.
|
|
Hi @Elpiro , sounds good. I can help you work through the CI test failures whenever you get back to this. The next step would be to merge with master and then push again and we can take a look at what is failing. |
|
Hi @Elpiro, just wanted to let you know that we're planning to release version 3.3.0 next week. No pressure at all, but this would be a nice addition if you have time to pick it up again. Otherwise it's fine to bump it to 3.4.0. Thanks! |
|
I updated the code with the correct indentation, which I think was causing
all the previous errors. It still fails on 1 test on circleCI. I will look
into it later.
|
|
Hi @Elpiro , to run the current test suite you'll need to merge in master and then push again. Then we can take a look at what might still be failing. Thanks! |
|
Hello @jonmmease and @Elpiro |
|
Thanks for the offer @paulamool, @Elpiro what do you think? |
|
Yes, I still have 1 error on circleci that won't to go away. So let's pull :) |
|
Completed in #1214 |
The threshold value was automatically computed from scipy's linkage function. Sometimes the automatic value is not the optimal value (too few clusters in my case)