This pull request represents a significant chunk of work to address a simple bug:
import matplotlib.pyplot as plt
import matplotlib.transforms as mtrans
ax = plt.axes()
off_trans = mtrans.Affine2D().translate(10, 10)
plt.plot(range(11), transform=off_trans + ax.transData)
print(ax.dataLim)
The result should be [10, 10, 20, 20], but the offset transform has not been taken into account.
Since a path transformation can be costly, it made sense to use the created Line's cached transform concept.
This threw up another, quite confusing bug:
import matplotlib.projections.polar as polar
import matplotlib.transforms as mtrans
import matplotlib.path as mpath
import numpy as np
full = mtrans.Affine2D().translate(1, 0) + polar.PolarAxes.PolarTransform()
verts = np.array([[0, 0], [5, 5], [2, 0]])
p = mpath.Path(verts)
tpath = mtrans.TransformedPath(p, full)
partial_p, aff = tpath.get_transformed_path_and_affine()
print full.transform_path_affine(full.transform_path_non_affine(p))
print full.get_affine().transform_path_affine(full.transform_path_non_affine(p))
The numbers themselves aren't important, suffice to say that the former is correct.
Additionally, the need for non-affine Transform subclasses to implement transform_non_affine and also copy this definition into transform too is confusing/obfuscating e.g.:
class PolarTransform(Transform):
def transform(self, tr):
# ...
# do some stuff
# ...
return xy
transform.__doc__ = Transform.transform.__doc__
transform_non_affine = transform
transform_non_affine.__doc__ = Transform.transform_non_affine.__doc__
This latter complaint is the result of an optimisation that will see little benefit (transform stacks are typically mostly Affine, and the non-affine part is easily cached).
Therefore this pull request represents a simplification (at the cost of a couple more function calls) of the current Transform framework. Whilst it is my opinion that the Transform class heirachy remains non-optimally representative of the problem space, I have tried to be pragmatic in my changes for both backwards compatibility and size of review considerations.
This pull request is independent of the invalidation mechanism upgrade being discussed in #723, and a merge between the two should be straight forward.
The tests run exactly the same as they did before commencing this work (they weren't passing on my machine in the first place, but the RMS values have not changed at all). The run time has gone up 5 seconds up to 458 seconds (~1% slower), but this includes the new tests as a result of this pull.
Note this change subtly affects the way one should implement a Transform. If you are implementing a non affine transformation, then you should
override the transform_non_affine, rather than overriding the transform & copying the transform into transform_non_affine too. e.g.:
class PolarTransform(Transform):
def transform_non_affine(self, tr):
# ...
# do some stuff
# ...
return xy
transform_non_affine.__doc__ = Transform.transform_non_affine.__doc__
The documentation is still representative of this change, hence there are few documentation changes included in this pull request.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request represents a significant chunk of work to address a simple bug:
The result should be [10, 10, 20, 20], but the offset transform has not been taken into account.
Since a path transformation can be costly, it made sense to use the created Line's cached transform concept.
This threw up another, quite confusing bug:
The numbers themselves aren't important, suffice to say that the former is correct.
Additionally, the need for non-affine Transform subclasses to implement transform_non_affine and also copy this definition into transform too is confusing/obfuscating e.g.:
class PolarTransform(Transform): def transform(self, tr): # ... # do some stuff # ... return xy transform.__doc__ = Transform.transform.__doc__ transform_non_affine = transform transform_non_affine.__doc__ = Transform.transform_non_affine.__doc__This latter complaint is the result of an optimisation that will see little benefit (transform stacks are typically mostly Affine, and the non-affine part is easily cached).
Therefore this pull request represents a simplification (at the cost of a couple more function calls) of the current Transform framework. Whilst it is my opinion that the Transform class heirachy remains non-optimally representative of the problem space, I have tried to be pragmatic in my changes for both backwards compatibility and size of review considerations.
This pull request is independent of the invalidation mechanism upgrade being discussed in #723, and a merge between the two should be straight forward.
The tests run exactly the same as they did before commencing this work (they weren't passing on my machine in the first place, but the RMS values have not changed at all). The run time has gone up 5 seconds up to 458 seconds (~1% slower), but this includes the new tests as a result of this pull.
Note this change subtly affects the way one should implement a Transform. If you are implementing a non affine transformation, then you should
override the transform_non_affine, rather than overriding the transform & copying the transform into transform_non_affine too. e.g.:
class PolarTransform(Transform): def transform_non_affine(self, tr): # ... # do some stuff # ... return xy transform_non_affine.__doc__ = Transform.transform_non_affine.__doc__The documentation is still representative of this change, hence there are few documentation changes included in this pull request.