GeezerVison

I’m not clear this is actually an improvement. It creates more indirection and makes the test harder to follow.
In fact, I’d claim self.single_point is an anti pattern. I’d rather have the value spelled out

Read: “r90 transforms [1, 1] approximately to [-1, 1]”

If you want to abstract from here, it should be something like

I also would not create separate check functions for multiple points.

Either explicitly spell this out as

or absorb the logic into _check_transform, i.e. detect if it has a single point passed and then additionally test the point wrapped in a sequence.

For multiple points, stating the inputs is a bit longer and may need wrapping, but I think that’s still preferable to a magic distant input.

I'm not a fan of self.single_point or self.multiple_points either, I think they make it hard to figure out what the test is actually doing since you have to find the top of the class in your editor. I wasn't sure what the team's preference was, however.

I like _check_transform and absorbing the logic for single point detection. If we do future optimizations (such as optimizing bbox or 2 points), that would also give us the ability to run both pathways as needed.

Are you ok with the exact keyword argument with a default of False and having some tests opt into assert_array_equal? If not, it would probably be easier to simply call assert_array_equal/assert_array_almost_equal, remove single_point/multiple_points, and add the "single point as 2D" cases manually.

I’m fundamentally ok with exact. I only don’t know what the criterion for (not) using exact is. It currently seems random. But I’m ok to accept the current choices as the status quo.

The existing use of assert_array_almost_equal is due to floating point imprecision.

For example, TestAffine2D.test_skew uses Affine2D().skew_deg(26.5650512, 14.0362435). Applying that to (1, 1) results in a point close to (1.5, 1.25), but not quite. If we were to set exact=True:

E       AssertionError: 
E       Arrays are not equal
E       
E       Mismatched elements: 2 / 2 (100%)
E       Mismatch at indices:
E        [0]: 1.5000000005000806 (ACTUAL), 1.5 (DESIRED)
E        [1]: 1.2500000005947753 (ACTUAL), 1.25 (DESIRED)
E       Max absolute difference among violations: 5.9477534e-10
E       Max relative difference among violations: 4.75820272e-10
E        ACTUAL: array([1.5 , 1.25])
E        DESIRED: array([1.5 , 1.25])

It would probably be better to change that specific test case to use math.degrees(math.atan(0.5)) rather than 26.5650512 and then use exact=True.

That works on my machine (arm64), but it may not be portable to other architectures, compilers, and/or versions of Python/numpy.

Sounds good to me!

I think the rotations around 90 degrees can't be exact due to the matrix containing values near 0:

>>> trans = t.Affine2D().rotate_deg_around(1.0, 1.0, 90).rotate_deg(180)
>>> trans
<matplotlib.transforms.Affine2D object at 0x1039f4590>
>>> trans.get_matrix()
array([[-1.8369702e-16,  1.0000000e+00, -2.0000000e+00],
       [-1.0000000e+00, -1.8369702e-16,  2.4492936e-16],
       [ 0.0000000e+00,  0.0000000e+00,  1.0000000e+00]])

The scale(3, -2).translate(23, 42) case probably should be exact; however, as you said, we can address that in a different PR.

@timhoffm - Since I touched a lot of the tests in this PR, would it be better to back those changes out and do a follow-up PR that only touches the tests?

No strong opinion. Whatever is less work. I’ve already approved. So there’s only one review left. If you can describe to / direct the second reviewer so that they can easily churn through the rest changes, that’d be good enough.

OTOH separating refactoring from optimization is generally good.

No strong opinion. Whatever is less work. I’ve already approved. So there’s only one review left. If you can describe to / direct the second reviewer so that they can easily churn through the rest changes, that’d be good enough.

You are the second reviewer :) Scott already approved this, minus the test case changes. I'm going to back out the test case refactoring and put that in a separate PR.