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

BUG: Fix dendrogram labeling, per #3822 #4920

Closed
wants to merge 5 commits into from

Conversation

jamestwebber
Copy link
Contributor

Fixing a bug (#3822) in which label formatting arguments to scipy.cluster.hierarchy.dendrogram were being ignored. Also, made the behavior of the formatting more consistent across the four orientation options. Unless user-specified, label size will be autoscaled based on the number of labels. For "top" and "bottom" oriented dendrograms, the labels will be rotated if there are a lot of them.

In my own tests this version has the desired behavior. Unfortunately plot outputs are a pain to write tests for, otherwise I would add one in.

After testing it seemed that it still wasn't working--annoyingly it appears that the order of the commands to matplotlib matters, as some formatting commands will overwrite/erase others.

Also, I added the resizing code to the other orientations. They're currently all based on the number of labels but that could be changed to the "p" parameter easily. This way seems simpler though.
@jamestwebber jamestwebber changed the title BUG: Fix dendrogram labeling, per #3822 BUG: Fix dendrogram labeling, per 3822 May 28, 2015
@jamestwebber jamestwebber changed the title BUG: Fix dendrogram labeling, per 3822 BUG: Fix dendrogram labeling, per #3822 May 28, 2015
@WarrenWeckesser
Copy link
Member

I noticed another (older) bug that you might as well fix while you are working on this. The code

if leaf_rotation:
    ... do something ...
else:
    ... do something else ...

will do the wrong thing if the user specifies leaf_rotation=0. The test should be if leaf_rotation is not None. Similarly, if leaf_font_size: should probably be if leaf_font_size is not None:, but I don't know what the point of setting leaf_font_size=0 would be.

@jamestwebber
Copy link
Contributor Author

Good point, fixed.

@jamestwebber
Copy link
Contributor Author

For what it's worth I think the failed test was just due to time--the same test passed in an earlier commit and all I changed was indentation.

@rgommers
Copy link
Member

@jamestwebber thanks for this PR. I've taken it over and built on top of it in gh-5164, so I'm closing this one.

A review of my PR would be very helpful.

@rgommers rgommers closed this Aug 16, 2015
@jamestwebber jamestwebber deleted the patch-1 branch June 7, 2017 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.cluster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants