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 axis keyword to dendrogram function #3201

Closed
wants to merge 4 commits into from

Conversation

jamestwebber
Copy link
Contributor

This fixes issue #1325 (Trac 798). It allows scipy dendrograms to easily be plotted on custom axes, e.g. as part of a larger figure. It does not import pylab at all if an axis is provided, which is useful for those who are using the matplotlib API.

This fixes issue 1325 (Trac scipy#798). It allows scipy dendrograms to easily be plotted on custom axes, e.g. as part of a larger figure. It does not import pylab at all if an axis is provided, which is useful for those who are using the matplotlib API.
@rgommers
Copy link
Member

Overall this looks like a good idea. A couple of comments:

The axis argument should be named ax, because that's how this argument is typically called. axis normally means the axis of the input array over which to apply a certain operation (np.mean(x, axis=1)).

A test should be added for this keyword. Matplotlib is an optional dependency, so the test should only run if MPL is installed. You can look at the stats.probplot test as an example for how to add such a test:
https://github.com/scipy/scipy/blob/master/scipy/stats/tests/test_morestats.py#L17
https://github.com/scipy/scipy/blob/master/scipy/stats/tests/test_morestats.py#L436

@rgommers
Copy link
Member

TravisCI failure is unrelated, can be ignored.

@jamestwebber
Copy link
Contributor Author

Changing from axis to ax is easy enough to do. 'axis' was used throughout the existing code so I went with it.

I wasn't sure how to test a plot, it looks like (in the example you pointed me to) there's no test for the correct output, just to make sure it doesn't fail? I can add that to the existing cluster tests.

Also changed "axis_gca" flag to be more descriptive "trigger_redraw".
@rgommers
Copy link
Member

Indeed, checking that it doesn't fail. Comparing graphics output is done by the MPL test suite, but is hard and overkill here.

Tests plotting to pylab.gca() and to an axis passed in as an argument.
@jamestwebber
Copy link
Contributor Author

The interface for dendrogram is slightly different than the one for probplot (and it seems out of scope to try to change that). I didn't compare the output of the two different calls because there isn't the option to pass in pyplot instead of an axis.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d229aa3 on jamestwebber:patch-1 into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d229aa3 on jamestwebber:patch-1 into * on scipy:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bf2ec22 on jamestwebber:patch-1 into * on scipy:master*.

rgommers added a commit that referenced this pull request Jan 18, 2014
@rgommers
Copy link
Member

Test and doc addition look good. PR merged in dc7555b. Thanks @jamestwebber

@rgommers rgommers closed this Jan 18, 2014
@rgommers
Copy link
Member

Note that I did edit your commit messages to prepend the standard abbreviations listed at http://docs.scipy.org/doc/numpy-dev/dev/gitwash/development_workflow.html#writing-the-commit-message.

@jamestwebber jamestwebber deleted the patch-1 branch January 18, 2014 19:46
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

3 participants