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

Added parameter to choose color_threshold #1075

Closed
wants to merge 2 commits into from
Closed

Conversation

Elpiro
Copy link

@Elpiro Elpiro commented Jul 27, 2018

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)

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stick with dict or use graph_objs.scatter.Marker. graph_objs.Marker is deprecated in version 3



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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

hovertext)
linkagefun,
hovertext,
color_threshold)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there no closing parenthesis here?



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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align with :param above

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

graph_objs.Data is deprecated in version 3. Just stick with dd_traces as a list or tuple.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please align with labels= above

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]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

graph_objs.Marker is deprecated. So this should either stay a dict, or be graph_objs.scatter.Marker. Given the dendrogram performance discussion in #1052, I think we should avoid using graph_objs here and stick with dicts. This would also be consistent with my goals described in #1079

@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick with dict (See below)

@jonmmease
Copy link
Contributor

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!

@Elpiro
Copy link
Author

Elpiro commented Aug 25, 2018 via email

@jonmmease
Copy link
Contributor

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.

@jonmmease
Copy link
Contributor

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!

@Elpiro
Copy link
Author

Elpiro commented Sep 19, 2018 via email

@jonmmease
Copy link
Contributor

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!

@paulamool
Copy link
Contributor

Hello @jonmmease and @Elpiro
I have a master/build plotly.py (including changes made by elpiro) in my fork which succeeded. Okay to add a new pull request?

@jonmmease
Copy link
Contributor

Thanks for the offer @paulamool, @Elpiro what do you think?

@Elpiro
Copy link
Author

Elpiro commented Oct 3, 2018

Yes, I still have 1 error on circleci that won't to go away. So let's pull :)

@jonmmease
Copy link
Contributor

Completed in #1214

@jonmmease jonmmease closed this Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants