Fix step histogram endline #2372
Conversation
|
👍 Any chance you could just use png for test? I don't think there's much benefit to testing all 3 here, and we're trying to limit the number of tests we accumulate over time. Or, better yet, is there way to modify an existing test to show off this problem? |
|
Hi, I'm actually not sure that the fix really solved the problem. The endline is now a bit too long and goes on the left, on the x axis. It is also present on the png you gave if you look closely. Cheers, Nicolas |
|
Ah, you are indeed correct. I'll have to dig into this a little more. The fancy indexing in this chunk of code is pretty hairy. |
|
A simple "+2" instead of "+1" works for me (split = int(len(x) / 2) + 2 ), but I did not check if this is a robust fix... |
|
Given the complexity, I think I need to go through and fully analyze that bit of code. The other thing that's been puzzling me is that the problem doesn't show up when you plot multiple datasets, as far as I can tell. |
|
Alright, after reloading the entire thing into my brain and drawing myself a picture, I figured out what was going on. This should be correct. I also fixed a couple other oddities which gave results that were incorrect, but not visibly so. @mdboom Any idea how I remove the .pdf and .svg from the test? The basic |
| @@ -1062,6 +1062,17 @@ def test_hist_offset(): | |||
| ax.hist(d2, bottom=15) | |||
|
|
|||
|
|
|||
| @image_comparison(baseline_images=['hist_step']) | |||
There was a problem hiding this comment.
Stick an extensions=['png'] in here so that we don't get extraneous images being generated. Also, if you wouldn't mind, stick in a remove_text=True too.
|
I removed the svg and pdf in the test. |
|
@neggert: Sorry I didn't get back to you sooner. I see you've figured it out in the meantime. ;) |
Fix #2365.
This was a simple off-by-one error (hat tip to @nicolaschotard for finding the bug as well as reporting it). I added a new test that should catch this if it pops up again.