Skip to content

Avoid single_path_optimization if offsets are non-zero #16517

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Feb 15, 2020

This is a stopgap to fix #16461 - I am guessing this is made redundant by #13696, but is a simpler fix and probably easier to get into 3.2.0.

@dstansby dstansby added this to the v3.2.0 milestone Feb 15, 2020
@anntzer
Copy link
Contributor

anntzer commented Feb 15, 2020

Perhaps do a test like given in #16461 but settings x/ylim so that in the correct case the entire patch is out of bounds, but would be drawn at (0, 0) otherwise, and then check_figures_equal() against an empty plot?

@dstansby
Copy link
Member Author

That worked well - the test I've added fails on current master but passes here.

jklymak
jklymak previously approved these changes Feb 15, 2020
@jklymak jklymak self-requested a review February 15, 2020 17:35
@jklymak jklymak dismissed their stale review February 15, 2020 17:35

Failing CI

@anntzer
Copy link
Contributor

anntzer commented Feb 15, 2020

If we need to change baseline images (especially non-hexbin-related ones) I'd really rather just go with #13696 -- this bug has apparently been present for a long time, I'm not convinced it's RC to have it fixed in 3.2. Or if it helps I can make VectorTransform public in #13696; I don't like it but it's not the end of the world.

@dstansby
Copy link
Member Author

Agree with not needing to put this in 3.2. Won't the baseline images have to change anyway at some point though?

@dstansby dstansby modified the milestones: v3.2.0, v3.3.0 Feb 15, 2020
@anntzer
Copy link
Contributor

anntzer commented Feb 15, 2020

I don't think so. I think (not 100% sure?) here you disabled single_path_optimization when offsets are nonzero, but they really only need to be disabled when offsets are nonzero and offset_position="data". When offsets are nonzero and offset_position="screen" (which will always be the case after #13696), the old baseline images are still fine.

@dstansby
Copy link
Member Author

Ah yes, true - I can check for offset_position = 'data' here and not worry about the baseline images then.

Surely going through either of the two code paths should give identical results, but it doesn't look like it does because the images have changed. I guess the question is for offset_position = 'screen', which code path is doing the right thing?

@dstansby dstansby reopened this Feb 15, 2020
@anntzer
Copy link
Contributor

anntzer commented Feb 15, 2020

Basically single_path_optimization is slightly "wrong", because it "rounds" the markers to the closest pixel (it's fundamentally the original reason I wrote mplcairo...), see e.g. #7233.
But there are cases, e.g. ticks, where the rounding is actually useful because if you try to draw short ticks antialiased "between" two rows of pixel, they look particularly bad.

Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@QuLogic
Copy link
Member

QuLogic commented Apr 27, 2020

So this is replaced by #13696?

@anntzer
Copy link
Contributor

anntzer commented Apr 27, 2020

I think so? feel free to reopen if not.

@anntzer anntzer closed this Apr 27, 2020
@QuLogic QuLogic removed this from the v3.3.0 milestone Apr 27, 2020
@dstansby dstansby deleted the avoid-optim branch April 28, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hexbin if singular and mincnt used
5 participants