Skip to content

Properly minimize the rasterized layers #5815

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 9, 2016
Merged

Properly minimize the rasterized layers #5815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec45630
Fix #5814
mdboom Jan 8, 2016
87d82ab
Add test
mdboom Jan 8, 2016
2c99e51
Remove unnecessary -1's
mdboom Jan 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions lib/matplotlib/tests/test_image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,40 @@ def test_jpeg_alpha():
assert image.getpixel((0, 0)) == (254, 0, 0)


@cleanup
def test_minimized_rasterized():
# This ensures that the rasterized content in the colorbars is
# only as thick as the colorbar, and doesn't extend to other parts
# of the image. See #5814. While the original bug exists only
# in Postscript, the best way to detect it is to generate SVG
# and then parse the output to make sure the two colorbar images
# are the same size.
from xml.etree import ElementTree

np.random.seed(0)
data = np.random.rand(10, 10)

fig, ax = plt.subplots(1, 2)
p1 = ax[0].pcolormesh(data)
p2 = ax[1].pcolormesh(data)

plt.colorbar(p1, ax=ax[0])
plt.colorbar(p2, ax=ax[1])

buff = io.BytesIO()
plt.savefig(buff, format='svg')

buff = io.BytesIO(buff.getvalue())
tree = ElementTree.parse(buff)
width = None
for image in tree.iter('image'):
if width is None:
width = image['width']
else:
if image['width'] != width:
assert False


if __name__=='__main__':
import nose
nose.runmodule(argv=['-s','--with-doctest'], exit=False)
8 changes: 4 additions & 4 deletions src/_backend_agg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,10 @@ agg::rect_i RendererAgg::get_content_extents()
}
}

r.x1 = std::max(0, r.x1 - 1);
r.y1 = std::max(0, r.y1 - 1);
r.x2 = std::max(r.x2 + 1, (int)width);
r.y2 = std::max(r.y2 + 1, (int)height);
r.x1 = std::max(0, r.x1);
r.y1 = std::max(0, r.y1);
r.x2 = std::min(r.x2 + 1, (int)width);
r.y2 = std::min(r.y2 + 1, (int)height);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird, I can't find when these lines were introduced. Git blame said they were last touched during the CXX refactor, but when I go to that commit, I can't find these lines anywhere...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to be called tostring_rgba_minimized. And actually it looks like it used to have this fix, which got transferred incorrectly in the C++ refactor:

       // Expand the bounds by 1 pixel on all sides
        xmin = std::max(0, xmin - 1);
        ymin = std::max(0, ymin - 1);
        xmax = std::min(xmax, (int)width);
        ymax = std::min(ymax, (int)height);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with the plus 1's and minus 1's?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The +1 makes sense to me. The -1 I'm not sure, but it's been there forever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, neither of them make sense to me. Instead, I think the -1 shouldn't be there for x1/y1, and instead of +1 on x2/y2, it should be -1 on width/height. Note, I am not entirely certain if the subsequent code assumes 0 or 1-based indexing. I presume it is 0-based.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's zero-based. The range is like a Python slice, where the lower value is inclusive and the upper value is exclusive. Hence the need for +1 on the upper values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so the -1's doesn't make sense at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, except for the original comment saying that it is trying to expand the bounds on all sides, but doesn't say why

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah -- we could take the -1's out and see what happens to the test suite. Now that we're not doing fuzzy testing, we shouldn't know pretty quickly if it has negative consequences.


return r;
}
Expand Down