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

Fixes issue #966: When appending the new axes, there is a bug where it #2735

Merged
merged 3 commits into from Mar 20, 2014

Conversation

cimarronm
Copy link
Contributor

does not account for the reference horizontal locator index when adding
a veritcal axes and does not accout for the reference vertical locator
index when adding a horizontal axes. This fixes that bug so that the
append_axes function adds the axes where it is expected.

@cimarronm
Copy link
Contributor Author

Extending the sample code in the issue a bit

import numpy as np
import matplotlib.pyplot as plt
from mpl_toolkits.axes_grid1 import make_axes_locatable

# the random data
x = np.random.randn(1000)
y = np.random.randn(1000)

fig, axScatter = plt.subplots(figsize=(5.5,5.5))

# the scatter plot:
axScatter.scatter(x, y)
axScatter.set_aspect(1.)

# create new axes on the right and on the top of the current axes
# The first argument of the new_vertical(new_horizontal) method is
# the height (width) of the axes to be created in inches.
divider = make_axes_locatable(axScatter)
axHistbot = divider.append_axes("bottom", 1.2, pad=0.1, sharex=axScatter)
axHistright = divider.append_axes("right", 1.2, pad=0.1, sharey=axScatter)
axHistleft = divider.append_axes("left", 1.2, pad=0.1, sharey=axScatter)
axHisttop = divider.append_axes("top", 1.2, pad=0.1, sharex=axScatter)

# now determine nice limits by hand:
binwidth = 0.25
xymax = np.ma
x( [np.max(np.fabs(x)), np.max(np.fabs(y))] )
lim = ( int(xymax/binwidth) + 1) * binwidth

bins = np.arange(-lim, lim + binwidth, binwidth)
axHisttop.hist(x, bins=bins)
axHistbot.hist(x, bins=bins)
axHistleft.hist(y, bins=bins, orientation='horizontal')
axHistright.hist(y, bins=bins, orientation='horizontal')

axHistbot.invert_yaxis()
axHistleft.invert_xaxis()

axHisttop.xaxis.set_ticklabels(())
axHistbot.xaxis.set_ticklabels(())
axHistleft.yaxis.set_ticklabels(())
axHistright.yaxis.set_ticklabels(())

plt.show()

Before the fix
image

After the fix
image

@WeatherGod
Copy link
Member

Errors are unrelated (smoketests for animations). If possible, I would like to see this example added as a test.

@mdboom
Copy link
Member

mdboom commented Jan 27, 2014

I agree with @WeaterGod: If we could get this added as a test, and maybe a note in whatsnew, I'm all for this!

@cimarronm
Copy link
Contributor Author

Is there a way to see the images generated by Travis? It seem like I might be able to access it at https://s3.amazonaws.com/matplotlib-test-results/artifacts/3470/3470.1/result_images.tar.bz2 based upon the log.

@tacaswell
Copy link
Member

Sort of. The results from builds of branches on the matplotlib organization repo, but not pull requests get uploaded to aws for security reasons.

I created a branch for you to get the tests back (matplotlib/cimarronm-divider_append_axes_fix)

@tacaswell
Copy link
Member

@tacaswell
Copy link
Member

@cimarronm Trying to triage issues for 1.4, has there been any progress on this?

@cimarronm
Copy link
Contributor Author

@tacaswell The test doesn't seem to be working on Travis (it has a blank plot) but works perfectly on my setup. I'll try to simplify the test and see if I can get it to work in Travis.

@cimarronm
Copy link
Contributor Author

I'm not sure why Travis seems to generate empty plots for the test case I added (at least based upon the images I see in the tarball from cimarronm-divider_append_axes_fix branch). I cannot reproduce the behavior locally but I'm guess there is some difference between the setups causing the issue.

@tacaswell, would you be able to look at the test case I added and see if you see anything which looks suspicious.

@tacaswell
Copy link
Member

I will take a look at it tonight.

@tacaswell
Copy link
Member

sorry, still have not gotten to this....

@tacaswell
Copy link
Member

First thing I tried was re-starting the travis tests which resulted in just 3.2 passing which is rather odd in general....

@tacaswell
Copy link
Member

Testing locally there is something funny going on with the parallel tests.

If I run just this test it passes, but when I run it using the parallel incantation (like we do on travis) it fails. Adding a plt.show() didn't help either.

@cimarronm Is there something in this code path that depends on the global state? I am not super familiar with mpl_toolkits.

@tacaswell
Copy link
Member

@cimarronm Could you also add a CHANGELOG and maybe a whats_new entry?

I am inclined to knowfail this test and create a new issue re the parallel test failure.

…s a bug where it

does not account for the reference horizontal locator index when adding
a veritcal axes and does not accout for the reference vertical locator
index when adding a horizontal axes. This fixes that bug so that the
append_axes function adds the axes where it is expected.
@cimarronm
Copy link
Contributor Author

@tacaswell I added an entry to the CHANGELOG. I wasn't sure where to add something for whats_new (let me know where you think it should go if you want to add something there).

Good find on the parallel testing. I was running as a single instance and didn't even think of that. I'll see if I can reproduce on my side and track down knowing that.

@tacaswell
Copy link
Member

Don't give me too much credit, I just happened to hit the full version of the test suite in my command history before I hit a single test entry 😉

If you couldn't find a natural place in whats_new don't worry about it. This looks like a rather show-stopper bug, so I suspect not many people were using this code path. Fixing the is morally equivalent to adding a new feature, it is nifty and should get some advertisement.

@cimarronm
Copy link
Contributor Author

@tacaswell Ok, I believe I have solved the problem with the tests. I was able to narrow down the issue to an interaction with test_axes tests. The main problem is that there are tests in test_axes which do not cleanup after themselves and are missing the cleanup decorator. With the random ordering of tests in the parallel run, sometimes you would see the interaction and sometimes not.

I created PR #2911 to fix test_axes.py. I think you should be able to apply that and then retest this PR and it should pass (at least it does with my setup running parallel testing)

@tacaswell
Copy link
Member

Merged the other PR and restarted travis on this on.

tacaswell added a commit that referenced this pull request Mar 20, 2014
Fixes issue #966: When appending the new axes, there is a bug where it
@tacaswell tacaswell merged commit aaa2970 into matplotlib:master Mar 20, 2014
@tacaswell
Copy link
Member

merged

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

5 participants