Skip to content

Single path optimisation for Collection w/ offsets broken #16496

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.

Sign up for GitHub

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

Jump to bottom
Closed
dstansby opened this issue Feb 13, 2020 · 14 comments
Closed

Single path optimisation for Collection w/ offsets broken #16496

dstansby opened this issue Feb 13, 2020 · 14 comments
Milestone
v3.3.0

Comments

@dstansby
Copy link
Member

dstansby commented Feb 13, 2020 •
edited
Loading

This is the root cause of #16461. The single path optimisation does not correctly take into account offsets:

matplotlib/lib/matplotlib/collections.py

Lines 354 to 362 in acc7ecb

if do_single_path_optimization:
gc.set_foreground(tuple(edgecolors[0]))
gc.set_linewidth(self._linewidths[0])
gc.set_dashes(*self._linestyles[0])
gc.set_antialiased(self._antialiaseds[0])
gc.set_url(self._urls[0])
renderer.draw_markers(
gc, paths[0], combined_transform.frozen(),
mpath.Path(offsets), transOffset, tuple(facecolors[0]))

As an example, this code creates two collections which differ only by their edgecolors, and have an offset of [0.5, 0.5]. In both cases the collection should be drawn at the offset, but only the collection with edgecolors=None is correct, because this does not follow the 'optimised' code path above.

import matplotlib.pyplot as plt
import matplotlib.collections as mcoll
import matplotlib.transforms as mtransforms
import numpy as np

polygon = np.array([[-1, -1], [-1, 1], [1, 1], [1, -1]]) * 0.1
offsets = np.array([0.5, 0.5])

fig, axs = plt.subplots(ncols=2)
for ax, edgecolors in zip(axs, [None, 'face']):
    collection = mcoll.PolyCollection(
        [polygon],
        edgecolors=edgecolors,
        linewidths=[1],
        offsets=offsets,
        transOffset=mtransforms.IdentityTransform(),
        offset_position="data"
        )
    ax.add_collection(collection)

plt.show()

Figure_1
@dstansby dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 13, 2020
@anntzer
Copy link
Contributor

anntzer commented Feb 14, 2020

FWIW I think offset_position="data" should be deprecated. See #13696, #16461 (comment).

@anntzer anntzer mentioned this issue Feb 14, 2020
Merged
6 tasks
@QuLogic
Copy link
Member

QuLogic commented Feb 15, 2020

Changes to hexbin aside, possibly related to #15946?

@anntzer
Copy link
Contributor

anntzer commented Feb 15, 2020

Not directly, I think? At least the issue here seems to be speficically about offset_position="data" and is fixed by #13696, but #15946 doesn't use offset_position="data" and appears unaffected by my pr.

@QuLogic
Copy link
Member

QuLogic commented Feb 18, 2020

Ah, I didn't realize offset_position was a collection thing.

@tacaswell
Copy link
Member

@dstansby RC for which version?

@tacaswell
Copy link
Member

ok, the fixes here seem to be:

in order of most correct to simplest. If we want to fix this for 3.2, we should go with the last one, if we want to target this at 3.3 we should go with the first one.

@anntzer
Copy link
Contributor

anntzer commented Feb 18, 2020

The problem of the last approach is that you'd need to change the baseline images, as discussed above.
Again, I think we should just postpone this to 3.3, this bug has likely been present for a long time.

@tacaswell
Copy link
Member

I missed #16517 earlier (which basically implements the last one).

Sorry, I am being very dense, why does that require regenerating test images?

@anntzer
Copy link
Contributor

anntzer commented Feb 18, 2020

Becauses images with and without single path optimization are different (because single path optimization is based on stamping which means snapping to nearest pixels, whereas without SPO, you don't snap).

@anntzer
Copy link
Contributor

anntzer commented Feb 18, 2020 •
edited
Loading

Ah but wait, this was resolved via #16517 (comment).
So I guess that's fine even for 3.2.

@dstansby dstansby added this to the v3.2.1 milestone Feb 19, 2020
@dstansby dstansby removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 19, 2020
@QuLogic
Copy link
Member

QuLogic commented Mar 14, 2020

This is as old as 2.0.2 at least. In 1.5.3, both paths are drawn at (0, 0), which I guess is a slight improvement. But this is a very long-standing bug.

@tacaswell tacaswell modified the milestones: v3.2.1, v3.2.2 Mar 14, 2020
@QuLogic QuLogic mentioned this issue Mar 14, 2020
Closed
@goretkin
Copy link
Contributor

goretkin commented Apr 6, 2020

Axes.scatter produces a PathCollection with offset_position='screen', but with offsets given in data coordinates, as far as I can see. So fixing this would break Axes.scatter, unless it was also fixed.

@QuLogic
Copy link
Member

QuLogic commented May 9, 2020

#13696 deprecated offset_position='data', so replacing:

        transOffset=mtransforms.IdentityTransform(),
        offset_position="data"

with

        transOffset=mtransforms.AffineDeltaTransform(ax.transData),

works.

I think we can close this now, as the code path will eventually get deleted?

@QuLogic
Copy link
Member

QuLogic commented Jun 15, 2020

This is fixed for 3.3, and it doesn't look like anyone's going to open a simpler PR for 3.2.2.

@QuLogic QuLogic closed this as completed Jun 15, 2020
@QuLogic QuLogic modified the milestones: v3.2.2, v3.3.0 Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

No branches or pull requests
5 participants
@tacaswell @QuLogic @goretkin @anntzer @dstansby