Skip to content

MixedModeRenderer non-72-dpi fixes & Pgf mixed rendering#1975

Merged
pwuertz merged 2 commits into
matplotlib:masterfrom
pwuertz:pgf_mixedrenderer
May 27, 2013
Merged

MixedModeRenderer non-72-dpi fixes & Pgf mixed rendering#1975
pwuertz merged 2 commits into
matplotlib:masterfrom
pwuertz:pgf_mixedrenderer

Conversation

@pwuertz

@pwuertz pwuertz commented May 4, 2013

Copy link
Copy Markdown
Contributor

I would like to add mixed mode rendering + test to backend_pgf for handling plots that are too complex for vectorized output (https://github.com/pwuertz/matplotlib-backend-pgf/issues/5).

A small fix was necessary since MixedModeRenderer assumes that the figure's dpi is always 72. I think I also fixed a bad indent at stop_rasterizing that was probably unnoticeable due to the fixed dpi assumption. According to a comment in the code @leejjoon might know if this is correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just spotted this one too.

@pelson

pelson commented May 7, 2013

Copy link
Copy Markdown
Member

👍 - looks good to me.

@pwuertz

pwuertz commented May 7, 2013

Copy link
Copy Markdown
Contributor Author

Ok, this looks VERY related to the PR you mentioned. You think this will be beneficial or does it interfere with the other patch?

@pelson

pelson commented May 8, 2013

Copy link
Copy Markdown
Member

Ok, this looks VERY related to the PR you mentioned.

This PR fixes a bug which #1185 does not. Namely: The figure dpi is reverted to 72 after using the MixedModeRasterizer. After that, there is an awful lot of overlap. I suggest that we focus on #1185 (with a keen eye from yourself) and once that is in, we look at implementing this fix.

How does that sound?

@leejjoon

leejjoon commented May 9, 2013

Copy link
Copy Markdown
Contributor

Just looking through the code, I don't think it will interfere with #1185 (or other backends).
@pwuertz, did you happen to check if the pdf output from the pgf backend is consistent with that from the pdf backend when rasterization is used?

The hard-coded dpi of 72 was because all the vector backend uses dpi of 72 internally. But, this seems not to be the case of pgf backend.

While the PR looks good to me, I will try some test sometime this weekends.

@pwuertz

pwuertz commented May 22, 2013

Copy link
Copy Markdown
Contributor Author

@leejjoon The test I included looks the same for backend_pdf and backend_pgf.

@mdboom

mdboom commented May 24, 2013

Copy link
Copy Markdown
Member

#2044 (which is a rebased #1185) is almost ready -- there's an unreproducible test failure there at the moment. But once that's done, maybe we could update this PR to be based on it? I'd like to get this in for 1.3.0 if at all possible.

@mdboom

mdboom commented May 24, 2013

Copy link
Copy Markdown
Member

Ok -- #2044 is in, so I think all that's left for you @pwuertz is to submit everything this does that it doesn't do. Thanks!

@pwuertz

pwuertz commented May 25, 2013

Copy link
Copy Markdown
Contributor Author

Rebased on new master, but a test for py2.6 failed, don't know if that is related to my PR. I can check that tomorrow.

pwuertz added a commit that referenced this pull request May 27, 2013
MixedModeRenderer non-72-dpi fixes & Pgf mixed rendering
@pwuertz pwuertz merged commit df75b0b into matplotlib:master May 27, 2013
@pwuertz pwuertz deleted the pgf_mixedrenderer branch May 27, 2013 12:54
@pwuertz

pwuertz commented May 27, 2013

Copy link
Copy Markdown
Contributor Author

Allright, the 2.6 test fail is not related to this PR, merging...

@pwuertz

pwuertz commented May 27, 2013

Copy link
Copy Markdown
Contributor Author

py2.6 now passes, but py3.3 throws an ImageComparisonFailure for matplotlib.tests.test_image.test_rasterize_dpi.test although the images seem to be identical. The diff image is completely black.

pwuertz added a commit that referenced this pull request May 28, 2013
MixedModeRenderer non-72-dpi fixes & Pgf mixed rendering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants