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'])deftest_hist_step_log_bottom():# check that bottom doesn't get overwritten by the 'minimum' on a# log scale histogramnp.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: nobottom, green:bottom < 1/base, red:bottom > 1/base):

The test runs and passes in place on my system, at least.

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?
On Jul 9, 2015 12:54 PM, "Jens Hedegaard Nielsen"notifications@github.com
wrote:

Thanks that looks good to me.

Perhaps is is worth setting the colours explicitly for the 3 sections to
make it easier to compare the code to the image?

Adding a reference to the this pr i.e.#4608
#4608 to the comment would
be useful too.

—
Reply to this email directly or view it on GitHub
#4608 (comment)
.

@WeatherGod: which gap? I have manually set the lower ybound to9e-3 to highlight thebottom=1e-2 call, if that's what you mean.

@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
expecting to see blue for it not having a bottom, before realizing that
this is the stepfilled histogram. So, I might be getting confused as to
what the expected behavior is there (I don't use stepfilled).

On Thu, Jul 9, 2015 at 1:32 PM, Duncan Macleodnotifications@github.com
wrote:

@jenshnielsenhttps://github.com/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.

—
Reply to this email directly or view it on GitHub
#4608 (comment)
.

On alog=True,histtype=step{filled} histogram thebottom is automatically set to1/base (ifnormed=False, weights=None), ornp.min(x[nonzero])/base otherwise. This PR fixes the bug whereby that default would over-ride thebottom specified by a user, even if non-zero and positive.

Perhaps thebottom for a log-scale hist might be best defaulted to some small positive number (1e-100) so long as that doesn't automatically set the orientation axis lower bound as well (which is the current behaviour). I'll leave that for another issue/PR.

@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.
The reason this looks odd is just that the last bin is so much smaller than the rest as it should be.

I hadn't appreciated that settingbottom would actually move the lower-end of the bar, i.e. scale the bars to have the same height with a different starting point, but it looks like I've stumbled across a bug and a fix in the mean time.

I was actually just wanting to make sure that theminimum can be set to something off the bottom of the chart, especially when usingweights. Anyways, if the tests are good, I will commit and wait for review.

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()

Thanks