Skip to content

ENH: optimize Collection non-affine transform to call transform once #11465

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

Closed
wants to merge 1 commit into from
Closed

ENH: optimize Collection non-affine transform to call transform once #11465

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions lib/matplotlib/collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def get_datalim(self, transData):
paths = self.get_paths()

if not transform.is_affine:
paths = [transform.transform_path_non_affine(p) for p in paths]
paths = self._non_affine_transform_paths(paths, transform)
Copy link
Contributor

Choose a reason for hiding this comment

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

We've gone here from calling transform_path_non_affine to (effectively) transform_non_affine. The former returns a new Path, which based on my reading would seem to offer possibly interpolate points along the path in the transformed coordinate system. The latter will not do this. Now, does anyone know if this is important?

Is it possible to instead combine the individual Paths into one big path and use transform_path_non_affine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, hummm. OK< I just looked at the default transform_path_non_affine but that can be overridden, so I think you're right that we need to call that instead....

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the default as well, and the line I'm concerned we're skipping is:

return Path._fast_from_codes_and_verts(x, path.codes,                                      
                 {'interpolation_steps': path._interpolation_steps,                                 
                  'should_simplify': path.should_simplify})

Your current implementation calls self.transform_non_affine(vertices), just like the default transform_path_non_affine, but it reuses the existing Path instances.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I may be over paranoid. It looks like _fast_from_codes_and_verts just sets the interpolation steps, it doesn't do any actual interpolation at that time.

Copy link
Member Author

@jklymak jklymak Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

matplotlib/lib/matplotlib/projections/geo.py

Lines 260 to 265 in b078643

def transform_path_non_affine(self, path):
vertices = path.vertices
ipath = path.interpolated(self._resolution)
return Path(self.transform(ipath.vertices), ipath.codes)
transform_path_non_affine.__doc__ = \
Transform.transform_path_non_affine.__doc__
Looks to me like the path changes length. Which makes life pretty hard because its hard to then split up the new path based on the length of the input path...

Copy link
Member Author

Choose a reason for hiding this comment

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

So, its possible to come up w/ an algorithm that does this if transform_path_non_affine uses only path.interpolated to get the higher-res path. But, I don't see any reason that an arbitrary user-supplied transform would be restricted to doing that method of interpolation.

Given that ambiguity, I think its impossible to come up with a foolproof algorithm for this. I suppose the special case of no interpolation could be checked for and would work.

I'll point out that @astrofrog's fix is obviously counting on the non-affine path transform not doing any interpolation - i.e. its doing linear dot connecting after the tranform. Certainly that'd be a disaster for a geographic non-affine transform (i.e. you want a straight line in lat/lon to be curved in the projection), but maybe is fine for astropy?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jklymak - ah I see - in our case we write the transforms we are using and we don't make use of interpolation for now so things should be fine. It would be pretty rare for the resolution of the contour map to be so low that there would be significant distortions between vertices of the path.

transform = transform.get_affine()
if not transOffset.is_affine:
offsets = transOffset.transform_non_affine(offsets)
Expand All @@ -213,6 +213,22 @@ def get_window_extent(self, renderer):
# cases other than scatter plot legend
return self.get_datalim(transforms.IdentityTransform())

def _non_affine_transform_paths(self, paths, transform):
"""
Sometimes there will be many paths, and the transform is
slow to run. This helper concatenates the paths, transforms,
the points, and then splits them back into the individual paths.
"""
vertices = np.vstack(path.vertices for path in paths)
pos = np.cumsum([len(x) for x in paths])
vertices = transform.transform_non_affine(vertices)
# repopulate paths
verts = np.split(vertices, pos[:-1])
for nn, verti in enumerate(verts):
paths[nn].vertices = verti
Copy link
Contributor

Choose a reason for hiding this comment

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

You're modifying paths in place; if you trace back, this is a reference to self._paths, which means you're modifying the original contours. That seems really bad.


return paths

def _prepare_points(self):
"""Point prep for drawing and hit testing"""

Expand All @@ -236,8 +252,7 @@ def _prepare_points(self):
offsets = np.column_stack([xs, ys])

if not transform.is_affine:
paths = [transform.transform_path_non_affine(path)
for path in paths]
paths = self._non_affine_transform_paths(paths, transform)
transform = transform.get_affine()
if not transOffset.is_affine:
offsets = transOffset.transform_non_affine(offsets)
Expand Down