Skip to content

Correct error bar cap orientation in polar plots #28905

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

Closed
wants to merge 77 commits into from

Conversation

kyracho
Copy link
Contributor

@kyracho kyracho commented Sep 28, 2024

PR summary

I'm still working on writing a test to validate the fix and would appreciate any help or suggestions on how best to approach it. Thanks in advance!

import matplotlib.pyplot as plt
import numpy as np

theta = np.arange(0, 2 * np.pi, np.pi / 8)
r = theta / np.pi / 2 + 0.5

fig = plt.figure(figsize=(10, 10))
ax = fig.add_subplot(projection='polar')
ax.set_theta_zero_location("N")
ax.set_theta_direction(-1)
ax.errorbar(theta, r, xerr=0.1, yerr=0.1, capsize=7, fmt="o", c="seagreen")
ax.set_title("Pretty polar error bars")
plt.show()
Screenshot 2024-09-28 at 2 42 06 PM

Original _axes.py:

https://github.com/kyracho/matplotlib/blob/bb68469d755bc04e0ddabff3f36908a9e2351423/lib/matplotlib/axes/_axes.py#L3768-L3779

New _axes.py:

https://github.com/kyracho/matplotlib/blob/210ddbac8c4cec253af6e65baf8803522db7a1bd/lib/matplotlib/axes/_axes.py#L3768-L3782

PR checklist

@kyracho kyracho closed this Sep 28, 2024
@kyracho kyracho reopened this Sep 28, 2024
@kyracho kyracho marked this pull request as draft September 28, 2024 23:37
@rcomer
Copy link
Member

rcomer commented Sep 29, 2024

Thanks for picking this up @kyracho! I think it might make sense to adapt your above example into an image comparison test.

@markovas1ljevic
Copy link

@kyracho I'd like to see if I can help with this issue in any way, unsure how to write a test for this but as @rcomer mentioned, the image comparison will probably be helpful, this is similar to what I implemented in the previous thread.

@kyracho kyracho force-pushed the errorbar_caps_bug branch from 9fe791c to 063cb88 Compare October 1, 2024 07:33
@kyracho
Copy link
Contributor Author

kyracho commented Oct 1, 2024

@rcomer Thanks! I’ve added the image comparison test based on that example. Let me know if there is anything you'd like me to adjust.

@vas1l Appreciate the offer! but I think I’ve got it covered for now.

@kyracho kyracho marked this pull request as ready for review October 1, 2024 07:57
@kyracho kyracho force-pushed the errorbar_caps_bug branch from 00461b7 to 11de4c5 Compare October 1, 2024 08:48
@QuLogic
Copy link
Member

QuLogic commented Oct 1, 2024

This is an improvement, but it is not perfect in that you can change the theta offset and direction after the error bars are added:

import matplotlib.pyplot as plt
import numpy as np

theta = np.arange(0, 2 * np.pi, np.pi / 8)
r = theta / np.pi / 2 + 0.5

fig = plt.figure(figsize=(10, 10))
ax = fig.add_subplot(projection='polar')
ax.errorbar(theta, r, xerr=0.1, yerr=0.1, capsize=7, fmt="o", c="seagreen")
ax.set_theta_zero_location("N")
ax.set_theta_direction(-1)
ax.set_title("Pretty polar error bars")
plt.show()

and then it is broken again:
Figure_1
However, I had not realized that we have a cut-out for polar plots in errorbar. In that case, we should grab the transShift transform (or one of the private ones that make it up), which contains theta offset and direction and will automatically update as a normal transform does.

@kyracho
Copy link
Contributor Author

kyracho commented Oct 1, 2024

@QuLogic

Thanks for the suggestion! I initially noticed the behavior too. I’ve tried using transShift transform as you recommended, like this:

 if self.name == 'polar':
            trans_shift = self.transShift
            for axis in caplines:
                for l in caplines[axis]:
                    for theta, r in zip(l.get_xdata(), l.get_ydata()):
                        # Apply the transShift transformation to adjust theta
                        transformed_coords = trans_shift.transform([theta, r])
                        adjusted_theta = transformed_coords[0]  # Extract the transformed theta

                        # Apply rotation based on the transformed theta
                        if axis == 'y':
                            rotation = mtransforms.Affine2D().rotate(adjusted_theta - np.pi / 2)
                        else:
                            rotation = mtransforms.Affine2D().rotate(adjusted_theta)

                        # Apply the marker style with the adjusted rotation
                        ms = mmarkers.MarkerStyle(marker=marker, transform=rotation)
                        self.add_line(mlines.Line2D([theta], [r], marker=ms, **eb_cap_style))
        else:
            for axis in caplines:
                for l in caplines[axis]:
                    self.add_line(l)

While it does contain the theta_offset and theta_direction and correctly adjusts theta, the issue I'm encountering is that it doesn’t retroactively apply to the error bars if ax.errorbar() is called before setting theta_zero_location or theta_direction.

The reason for this is that while transShift dynamically updates for other plot elements, it doesn’t retroactively change error bars unless ax.errorbar() is called again or the error bars are manually updated.

Let me know if you have any thoughts or further suggestions on how we could potentially address this!

@kyracho kyracho force-pushed the errorbar_caps_bug branch 2 times, most recently from 5185c4d to 2b7ac42 Compare October 1, 2024 21:09
@timhoffm
Copy link
Member

timhoffm commented Oct 1, 2024

General note:

I haven't looked into the update mechanism and how difficult it is to get always consistent updates.

The one-time fix in which errorbar() accounts for the current offset and direction is already a good improvement. It allows to create the correct graphs (with the right call order). If we can't get the consistent updates to work right now, I'd be willing to accept the on-time calculation and open a separate issue for the consistent update.

@kyracho kyracho force-pushed the errorbar_caps_bug branch 2 times, most recently from 6846653 to 62325e9 Compare October 1, 2024 21:50
@rcomer rcomer mentioned this pull request Oct 3, 2024
@QuLogic
Copy link
Member

QuLogic commented Oct 4, 2024

While it does contain the theta_offset and theta_direction and correctly adjusts theta, the issue I'm encountering is that it doesn’t retroactively apply to the error bars if ax.errorbar() is called before setting theta_zero_location or theta_direction.

The reason for this is that while transShift dynamically updates for other plot elements, it doesn’t retroactively change error bars unless ax.errorbar() is called again or the error bars are manually updated.

This is because you are transforming theta with transShift, but not attaching transShift to the marker in any way, so there is no way for it to update. The tricky thing here is that we need to transform a value and then use that value to set another transform. Almost all of our transforms just take in values and output transformed ones. The closest we have to what we need here is ScaledTranslation, which applies a transform on two values and then is a translational transform using the resulting values.

So in order to get the runtime changes, we'd have to copy ScaledTranslation into a ScaledRotation, which would apply transShift to theta, and then be a rotational transform from the results. This would then be the transform passed to the marker. However, I have not worked out exactly how this would work on polar plots, as the transform there is non-separable. It may not be straightforward to create a ScaledRotation that works generically but also in polar coordinate spaces.

@kyracho
Copy link
Contributor Author

kyracho commented Oct 5, 2024

@QuLogic thanks!

Following feedback, I wrote the OffsetRotation class in transforms.py based on ScaledTranslation. The class constructs a rotation matrix, which rotates the error bar caps according to the offset and direction values. And I updated _axes.py to use OffsetRotation for transforming the error bar caps.

The error bars now update correctly when ax.set_theta_zero_location and/or ax.set_theta_direction are called after ax.errorbar.

I would appreciate any help or suggestions on passing the CI/CD tests and ensuring the code is optimal. Thanks in advance!

Comment on lines 3018 to 3020
# Adjust theta based on theta_zero_location and theta_direction
adjusted_theta = self._theta_direction * (
transformed_theta - self._theta_zero_location)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this; is it not undoing what trans_shift.transform did?

Copy link
Contributor Author

@kyracho kyracho Oct 5, 2024

Choose a reason for hiding this comment

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

Hi @QuLogic, I apologize for the confusion and circular logic in my previous push. While it technically worked, I was reapplying transformations unnecessarily. Embarrassing! I’ve since resolved this and cleaned up the approach to ensure the transformations are handled correctly, without redundant steps.

@kyracho
Copy link
Contributor Author

kyracho commented Oct 10, 2024

@QuLogic I renamed OffsetRotation to ScaledRotation as described in your comments earlier. I’m sorry for any confusion this may have caused. The fix for the dynamic update of the error bar caps were based on your suggestions. Thanks for your understanding!

Also, I wanted to apologize for any confusion caused by my messy commit history. I recent merged main into the PR branch instead of rebasing the PR branch on top of main. This has made it more difficult to squash my commits cleanly, and I realize that it's caused unnecessary complexity in the commit history. I'm working on cleaning it up, and I appreciate your patience while I get things sorted. Moving forward, I’ll be sure to rebase to avoid this issue.

@kyracho
Copy link
Contributor Author

kyracho commented Oct 10, 2024

Hi, I wanted to let you know that while attempting to squash my own commits, I accidentally introduced a bunch of commits that are not my own into the PR. Unfortunately, I haven't been able to return to the previous state before the squash.

To keep things clean and avoid further complications, I’m going to create a new branch and open a new PR. I’ll move over my changes and ensure that the commit history is correct in the new PR. Sorry for the confusion and thanks for your patience!

@kyracho kyracho deleted the errorbar_caps_bug branch November 3, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment