Properly minimize the rasterized layers by mdboom · Pull Request #5815 · matplotlib/matplotlib

mdboom · 2016-01-08T17:52:50Z

The creation of the bounding box of actual image content is wrong.

mdboom · 2016-01-08T17:53:21Z

@tacaswell: Your discretion whether to put this on 1.5.1rc or 1.5.2...

WeatherGod · 2016-01-08T18:13:30Z

src/_backend_agg.cpp

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...

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

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

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

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.

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.

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

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

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.

WeatherGod · 2016-01-08T18:16:21Z

hmm, the failure on 2.7 looks spurious, but it isn't one of the usual suspects (one of the stix tests). Restarted anyway.

WeatherGod · 2016-01-08T18:28:43Z

And Travis failed again, but on a completely different test this time. Restarted.

WeatherGod · 2016-01-08T19:22:18Z

Hmm, no failures in the Travis tests. Weird, but ok... waiting for appveyor.

WeatherGod · 2016-01-08T19:23:53Z

I think this should go into 1.5.1 since it fixes a pretty bad error, but I'll let @tacaswell decide if it is worth putting out a rc2 for.

tacaswell · 2016-01-08T19:45:39Z

👍 to putting this in 1.5.1 👎 on needing an rc2.

efiring · 2016-01-08T19:49:59Z

On 2016/01/08 9:45 AM, Thomas A Caswell wrote:

👍 to putting this in 1.5.1 👎 on needing an rc2.

Agreed!

WeatherGod · 2016-01-08T21:32:27Z

appveyor failed with weird errors. I don't think I have the ability to restart them.

Properly minimize the rasterized layers

…layer Properly minimize the rasterized layers Conflicts: lib/matplotlib/tests/test_image.py One too-many tests where cherry-picked

…layer Properly minimize the rasterized layers Conflicts: lib/matplotlib/tests/test_image.py One too-many tests were cherry-picked

Merge pull request #5815 from mdboom/fix-minimizing-raster-layer

tacaswell · 2016-01-10T20:22:58Z

backported via PR + 2 commits as ac8cfb3 in #5817

tomspur · 2016-01-12T21:48:03Z

With the current master, I get 4 failures and git bisect blames the latest commit in this pull request.
One example is matplotlib.testing.decorators.test_surface3d.test and the difference of the expected and generated picture looks like:

It seems the right end side of the image is not cropped correctly.

The other failing tests are:

matplotlib.tests.test_streamplot.test_colormap
matplotlib.tests.test_image.test_rasterize_dpi
matplotlib.tests.test_bbox_tight.test_bbox_inches_tight_raster

The following "patch" would fix the first two failing tests, but the other two tests are not 100% identical and still fail:

diff --git a/src/_backend_agg.cpp b/src/_backend_agg.cpp
index e472156..3b9742a 100644
--- a/src/_backend_agg.cpp
+++ b/src/_backend_agg.cpp
@@ -210,10 +210,10 @@ agg::rect_i RendererAgg::get_content_extents()
         }
     }

-    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);
+    r.x1 = std::max(0, r.x1 - 1);
+    r.y1 = std::max(0, r.y1 - 1);
+    r.x2 = std::min(r.x2 + 2, (int)width);
+    r.y2 = std::min(r.y2 + 2, (int)height);

     return r;
 }

Do you know, why/how this test could fail here (on Fedora), but not on travis? What else could I do to help debugging this?

tacaswell · 2016-01-12T22:14:12Z

Is this before or after #5834 went in?

Which formats are failing? If it is all svg, part of the issue may be the rasterization via inkscape.

tomspur · 2016-01-12T22:42:27Z

The latest commit from #5834 (and the whole v1.5.x branch) seems to work just fine.
Yet, the problem is in the master branch and all failing tests are indeed svg.

tacaswell · 2016-01-12T22:44:07Z

Oh, we need to merge 1.5.x back into master.

On Tue, Jan 12, 2016 at 5:42 PM Thomas Spura notifications@github.com
wrote:

The latest commit from #5834
#5834 (and the whole
v1.5.x branch) seems to work just fine.
Yet, the problem is in the master branch and all failing tests are indeed
svg.

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

mdboom added 2 commits January 8, 2016 12:39

Fix matplotlib#5814

ec45630

The creation of the bounding box of actual image content is wrong.

Add test

87d82ab

mdboom added this to the Critical bugfix release (1.5.1) milestone Jan 8, 2016

mdboom added the status: needs review label Jan 8, 2016

WeatherGod reviewed Jan 8, 2016
View reviewed changes

Remove unnecessary -1's

2c99e51

tacaswell added a commit that referenced this pull request Jan 9, 2016

Merge pull request #5815 from mdboom/fix-minimizing-raster-layer

90455d8

Properly minimize the rasterized layers

tacaswell merged commit 90455d8 into matplotlib:master Jan 9, 2016

tacaswell removed the status: needs review label Jan 9, 2016

tacaswell mentioned this pull request Jan 9, 2016

Merge pull request #5815 from mdboom/fix-minimizing-raster-layer #5817

Merged

tacaswell added a commit that referenced this pull request Jan 10, 2016

Merge pull request #5817 from tacaswell/backport

ac8cfb3

Merge pull request #5815 from mdboom/fix-minimizing-raster-layer

Uh oh!

Conversation

mdboom commented Jan 8, 2016

Uh oh!

mdboom commented Jan 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeatherGod commented Jan 8, 2016

Uh oh!

WeatherGod commented Jan 8, 2016

Uh oh!

WeatherGod commented Jan 8, 2016

Uh oh!

WeatherGod commented Jan 8, 2016

Uh oh!

tacaswell commented Jan 8, 2016

Uh oh!

efiring commented Jan 8, 2016

Uh oh!

WeatherGod commented Jan 8, 2016

Uh oh!

tacaswell commented Jan 10, 2016

Uh oh!

tomspur commented Jan 12, 2016

Uh oh!

tacaswell commented Jan 12, 2016

Uh oh!

tomspur commented Jan 12, 2016

Uh oh!

tacaswell commented Jan 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants