FIX: round instead of truncate Agg canvas size #8265
Conversation
| @@ -243,7 +243,7 @@ def get_renderer(self, cleared=None): | |||
| # Mirrors super.get_renderer, but caches the old one | |||
| # so that we can do things such as produce a diff image | |||
| # in get_diff_image | |||
| _, _, w, h = self.figure.bbox.bounds | |||
| _, _, w, h = np.round(self.figure.bbox.bounds).astype(int) | |||
| w, h = int(w), int(h) | |||
There was a problem hiding this comment.
This conversion to python int is not done in the other two backends, so presumably it is not needed here. (I see that numpy.int64 and int instances both hash to the corresponding python int.)
|
I get one failure locally now:
which is due to some text shifting vertically, not really sure what is going on here. The logic inside of |
|
FWIW, this doesn't fix the floating point round-off issues in the animation framework that #8253 resolves. I will try to make a simple test script to attach to that issue. |
|
@ngoldbaum, is it possible that using rounding consistently--here and in #8253, in place of int with the nextafter adjustment--would solve the problem? |
|
I don't know - all I tried was running my test yt script with this PR applied in my working copy. It failed, but it's possible there needs to be other code changes in the animation module. |
|
Thanks for the PR. This hasn't seen action for a long time so marking stale. Note that PRs sometimes fall through the cracks, so if this is important, please ping for more reviews. |
0324a6b to
08a91cf
Compare
153b637 to
98acc4f
Compare
This avoids marking the figure as stale un-necessarily. Also remove a local variable which is immediately replaced below.
The change to rounding made these images one pixel wider
When rasterizing the figure, the allowed sizes are discrete due to a finite dpi. When getting the renderer (at which point we have committed to rasterizing the figure at this dpi) feedback the actual size.
98acc4f to
7acb303
Compare
This still needs some work and I think tweak around the short-circuit on the resize is not working quite right either. |
|
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
|
It seems past time for something like this to get in; let's keep it alive. |
As suggested by @njsmith in #8253 round, rather than truncate the size in pixels of the Agg canvas. This fails two tests for me locally
iirc the second one is also unstable on travis. Do not have time to investigate further tonight, but regenerating two images seems like a small price to pay for making it more reliable to get renders of a fixed pixel size.