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

fix wrong leaves determination bug #1186

Merged
merged 3 commits into from
Sep 25, 2018
Merged

fix wrong leaves determination bug #1186

merged 3 commits into from
Sep 25, 2018

Conversation

444thLiao
Copy link
Contributor

@jonmmease Sorry for multiple pull request beacuse I am new to github cooperation.

Here is the formal request with code and example.

Before, run the code below

import plotly
import plotly.figure_factory as ff
import pandas as pd
demo_data = pd.DataFrame([[1,2,3,4],[1,2,3,4],[1,3,5,6],[1,4,2,3]])
figure = ff.create_dendrogram(demo_data)
plotly.offline.plot(figure)

demo_data looks like
image
The data is some simple data and it mainly simulated the situation which has three and more rows are identical. When we using the euclidean distance, the distance between 0,1 and 3 will be identical.

we could get the dendrogram like that
wrong dendrogram

In the graph above, we can see wrong multiple x-axis ticks text raised. It will be severely when we pass the leaves name. 35 in the graph is the original x-coordinate of the leaves.

After, run the same code above.
after bug fixed

Relative issue and request:
#1180
#1181

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.

Thanks for the fix @444thLiao! Your approach looks sound.

In addition to the small comments below, could you turn your example in the pull request description into a test?

To do this, add a new method to the TestDendrogram class in plotly/tests/test_optional/test_figure_factory/test_figure_factory.py and follow the pattern of the other tests in that class.

Thanks!

@@ -140,7 +140,10 @@ def __init__(self, X, orientation='bottom', labels=None, colorscale=None,
for i in range(len(yvals_flat)):
if yvals_flat[i] == 0.0 and xvals_flat[i] not in self.zero_vals:
self.zero_vals.append(xvals_flat[i])

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 add a nice comment here explaining why this check is here. (e.g. the situation with multiple identical values)

if len(self.zero_vals) > len(yvals) + 1:
l_border = int(min(self.zero_vals))
r_border = int(max(self.zero_vals))
self.zero_vals = [v for v in range(l_border,r_border + 1, int((r_border-l_border) / len(yvals)))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please break this line up into two lines so that it's not so wide. Maybe create a new local variable for the range(...) expression.

@jonmmease jonmmease added this to the v3.3.0 milestone Sep 19, 2018
@444thLiao
Copy link
Contributor Author

I have added comments and add a test function at suitable places. Maybe now it is suitable to merge into the master branch.

@jonmmease
Copy link
Contributor

jonmmease commented Sep 20, 2018

Thanks @444thLiao! I think these two test failures are due to the release of matplotlib 3.0, once I confirm that I'll merge these changes in. This will be released in version 3.3.0 in a week or so.

@jonmmease
Copy link
Contributor

@444thLiao Almost there. Matplotlib tests are fixed on master. Please merge master once more and push again. Thanks!

Add to/from/read/write json functions to the plotly.io module (#1188)
@444thLiao
Copy link
Contributor Author

Does it have some problems? I am not familiar with the circleci. @jonmmease

@jonmmease
Copy link
Contributor

I restarted the test and everything look good. Thanks for the contribution @444thLiao!

@jonmmease jonmmease merged commit fac00f1 into plotly:master Sep 25, 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.

None yet

2 participants