Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: round instead of truncate Agg canvas size#8265
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to08a91cfCompare153b637 to98acc4fCompareThis 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 afinite dpi. When getting the renderer (at which point we havecommitted to rasterizing the figure at this dpi) feedback the actualsize.
98acc4f to7acb303CompareThis 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.