Fix for issue #841

[ycopin]

As I mentioned in Issue #841, there's a bug in hist autoscaling when histtype.startswith('step). An easy test is:

import pylab as P
P.hist([3000,3010,3012], histtype='step')
P.show()

[dmcdougall]

Why not just np.min(n)?

Also, you don't need to refer to the issue it fixes.

[ycopin]

Why not just np.min(n)?

This would work as well, but for a small number of bins (default is 10), the min of mins is faster.

[pelson]

With such a small loop, performance isn't really a big concern, and readability becomes the leading decision aid. For me the numpy.min approach provides a certain amount of clarity as to what is being done, where the min( min(m) for m in n ) needs to be read twice to be sure you haven't missed anything.

As minor as it sounds, IMHO numpy.min would be preferable.

Cheers,

[dmcdougall]

Thanks for putting together a pull request for this. The time you've put in is much appreciated. Cheers.

This fixes a pretty annoying bug. Ideally, you should branch off of the v1.2.x branch instead of master. Also, it would be nice to have a test for this.

[dmcdougall]

Also, with your patch I get different results with log=True. This concerns me.

Edit: Comparing with v1.2.x

[ycopin]

• I'm sorry I don't know how to branch off the v1.2 branch (I'm not that familiar with git[hub]...)
• Which kind of test should I write? (I'm not familiar with tests...) Something like:

import pylab as P
P.hist([3000,3010,3012], histtype='step')
assert gca().get_ylim()==(0,1)

• What kind of different results with log=True do you obtain? My test case if of course not suited for log-scale; since bins are populated by 0 or 1 element only. An example with more elements looks fine to me:

import pylab as P
P.hist(P.random_integers(3000,3010, 100), histtype='step', log=True)
P.show()

[mdboom]

Don't worry to much about basing off of 1.2.x. We can always cherry-pick this onto that branch if we all agree it should go there.

[dmcdougall]

@mdboom It's a bug fix with a very simple/elegant solution. I'd be happy to see this in 1.2, but if others feel differently then I'm happy to back down.

There needs to be a test first, though, I think.

@ycopin The test should produce a plot that is incorrect before your fix is applied, but is correct after your fix is applied. This is to demonstrate that: a) your patch works; and b) if something breaks autoscaling for step-type histograms in the future your test will fail and a bug will be easier to find. The assert statement is used for tests that are not visual. I think a visual test is good here. For a guide on writing tests for matplotlib, check out this. Of course, non-image-based tests are also fine if you think that's a better indicator of the problem. I'd say that image comparison tests are to test for plots the 'look' wrong, and assertion based tests are to check the underlying number-crunching is done correctly. I defer to one of the other developers for which is better in this case.

Also, take a look at the lib/matplotlib/tests directory if you're struggling for inspiration.

[ycopin]

As you said, this PR is really just a very simple bug correction, not a new feature or anything fancy: the original line minimum = min(bins) is just wrong (a typo). So I wonder if this PR really needs a test...

[dmcdougall]

@ycopin A test certainly can't hurt.

[WeatherGod]

@ycopin Think of it this way. If we had a test that exercised that code, we would have detected this earlier. The test sample you gave above should be sufficient to catch this sort of mistake ever happening again (called a "regression"). Matplotlib has had a very poor test coverage over the years, and we are now trying to turn that around.

[efiring]

It's time to get this in. The test can wait. I am closing this in favor of an identical change based on v1.2.x.

[ycopin]

Thanks. I'll try to do better next time (not branching from master, include a test).

[dmcdougall]

@ycopin You can branch from v1.2.x by doing:

$ git fetch upstream  # get up to date
$ git checkout -b my_fix upstream/v1.2.x  # create a new branch called 'my_fix' and make it the current branch