-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix polar error bar cap orientation #28966
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
Conversation
a12c70e
to
dd8a598
Compare
dd8a598
to
604cb9f
Compare
9d5202a
to
912f347
Compare
912f347
to
4abf666
Compare
Hello, thanks so much for your feedback! I’ve incorporated the changes as suggested. Does everything look okay now? |
Yes, is there a reason it's still draft? |
Thanks for confirming! I marked it as ready for review just now. |
trans_shift.transform.assert_called_once_with([[theta, 0]]) | ||
expected_rotation = np.array([[0, -1], | ||
[1, 0]]) | ||
assert matrix is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert matrix is not None |
Can be left out. According to typing, this is always a nd.arrray. And even if we don't trust that completely, matrix[:2, :2]
in the next line would fail the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Can be merged after addressing the above small comment.
Merged over that small comment because that assert is cheap and we are looking to close out 3.10 milestone |
PR summary
Hello, This PR deals with #28885
When using
set_theta_direction
orset_theta_zero_location
in polar graphs, the error bar caps are not oriented correctly. We can transform the error bar caps using thetransShift
object. However, using the transShift object directly for transformations results in only a static transformation, rather than one that updates dynamically with the graph.The
ScaledRotation
class was suggested, that allows for dynamic updating of the error bar caps. This class is similar to theScaledTranslation
class and currently acceptstheta
,andis_radial
,self.transShift
as inputs. It outputs a3x3
transformation matrix, which can dynamically adjust the orientation of the caps.The
r
value (the radial coordinate) is not considered here because it is not necessary for this particular transformation.The get_matrix method computes the matrix using the
Affine2D
object, as Matplotlib requires this method to return a3x3
matrix.transforms.py
:And the
errorbar
function creates an instance of theScaledRotation
class for each of the cap lines. The only change in_axes.py
is creating instances of theScaledRotation
class, which are used to rotate the error bar caps._axes.py
:I noticed that
the dynamic updates only happen when thethe caps do not render dynamically when theScaledRotation
instance is passed directly tommarkers.MarkerStyle
. Andadjusted_theta
value is extracted from the class and then passed tommarkers.MarkerStyle
. I don't know how the Matplotlib transformation pipeline works in detail.And the logic for rotating the radial caps by
-pi/2
needs to be included afterself._trans_shift.transform
.but before the instance is passed tommarkers.MarkerStyle
. As a result, I was unable to move out the code for rotating the radial caps, which would make the class more modular.I still don't know if this is the right fix, but
transforms.piy
:An image comparison test:
Examples 1:
Example 2:
PR checklist