-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Axes.hist: use bottom for minimum if log and histtype='step...' #4608
Axes.hist: use bottom for minimum if log and histtype='step...' #4608
Conversation
The fix looks correct to me even if the remaining code is more complicated than I would like. Could you add a couple of tests for this? I would add a test where bottom is larger than 1/base and one where it is smaller. http://matplotlib.org/devel/testing.html#writing-an-image-comparison-test describes how to do image comparison tests. We probably only need a png test. |
@jenshnielsen: I have drafted the following test, but wanted to run it by you before committing a new image to the repo: @image_comparison(baseline_images=['hist_step_log_bottom'],
remove_text=True, extensions=['png'])
def test_hist_step_log_bottom():
# check that bottom doesn't get overwritten by the 'minimum' on a
# log scale histogram
np.random.seed(0)
data = np.random.standard_normal(2000)
fig = plt.figure()
ax = fig.add_subplot(111)
ax.hist(data, log=True, histtype='stepfilled', alpha=0.5)
ax.hist(data, log=True, histtype='stepfilled', bottom=1e-2, alpha=0.5)
ax.hist(data, log=True, histtype='stepfilled', bottom=0.5, alpha=0.5)
ax.set_ylim(9e-3, 1e3) which produces (blue: no |
Thanks that looks good to me. Perhaps is is worth setting the colours explicitly for the 3 hist plots to make it easier to compare the code to the image? Adding a reference to the this pr i.e. #4608 to the comment would be useful too. |
Does the gap at the bottom make sense?
|
@WeatherGod: which gap? I have manually set the lower ybound to |
@jenshnielsen: I can set the colours explicitly, my assumption was that the image is only every analysed by the test engine, and not by real humans. Anyways, simple, done. |
I was seeing the gap between the green bar and the x-axis, and was On Thu, Jul 9, 2015 at 1:32 PM, Duncan Macleod notifications@github.com
|
On a Perhaps the |
@duncanmmacleod Yes it's not at all critical. It just makes it a bit easier if the test breaks to understand what the intention of the test is |
It looks like there is another bug with the snapping of the top of the bars not being exactly identical see the last bar. |
Nevermind that is just bottom working as expected offsetting the histogram. |
I hadn't appreciated that setting I was actually just wanting to make sure that the |
Neither had I. I think we should clarify the docstring but that is for another time. Your test is good so please go ahead and commit it 👍 Perhaps it is worth adding a test where bottom is an array too. Something like the following should work: np.random.seed(0)
data = (np.random.standard_normal(2000))
bottom = np.arange(1,11)
fig = plt.figure()
ax = fig.add_subplot(111)
hist1 = ax.hist(data, log=True, histtype='stepfilled', bottom=bottom, alpha=0.5, color='green')
hist2 = ax.hist(data, log=True, histtype='stepfilled', alpha=0.5, color='blue')
ax.set_ylim(9e-3, 1e3)
plt.show() |
- this new test checks that the `bottom` kwarg to `Axes.hist` behaves as expected under a variety of circumstances when `log=True, histtype='step{filled}' given
Axes.hist: use bottom for minimum if log and histtype='step...'
Thanks |
This PR fixes #4606, whereby a histogram with
log=True
andhisttype=step...
would overridebottom
to prevent zeros on a log scale.With the included patch, the same code (or at least very similar) as given in the issue produces the following: