Skip to content

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

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

kyracho
Copy link
Contributor

@kyracho kyracho commented Oct 11, 2024

PR summary

Hello, This PR deals with #28885

When using set_theta_direction or set_theta_zero_location in polar graphs, the error bar caps are not oriented correctly. We can transform the error bar caps using the transShift 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 the ScaledTranslation class and currently accepts theta, is_radial, and self.transShift as inputs. It outputs a 3x3 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 a 3x3 matrix.

transforms.py:

class ScaledRotation(Affine2DBase):
    """
    A transformation that applies offset and direction
    based on *trans_shift*.
    """
    def __init__(self, theta, trans_shift, is_radial):
        super().__init__()
        self._theta = theta
        self._trans_shift = trans_shift
        self._is_radial = is_radial
        self._mtx = np.identity(3)

    def get_matrix(self):
        if self._invalid:
            transformed_coords = self._trans_shift.transform([[self._theta, 0]])[0]
            adjusted_theta = transformed_coords[0]
            if self._is_radial:
                adjusted_theta -= np.pi / 2
            rotation = mtransforms.Affine2D().rotate(adjusted_theta)
            self._mtx[:2, :2] = rotation.get_matrix()[:2, :2]
        return self._mtx

And the errorbar function creates an instance of the ScaledRotation class for each of the cap lines. The only change in _axes.py is creating instances of the ScaledRotation class, which are used to rotate the error bar caps.

_axes.py:

if self.name == 'polar':
            trans_shift = self.transShift
            for axis in caplines:
                for l in caplines[axis]:
                    # Rotate caps to be perpendicular to the error bars
                    for theta, r in zip(l.get_xdata(), l.get_ydata()):
                        rotation = ScaledRotation(
                            theta=theta,
                            trans_shift=trans_shift,
                            is_radial=(axis == 'y')
                        )
                        ms = mmarkers.MarkerStyle(marker=marker,
                                                  transform=rotation)
                        self.add_line(mlines.Line2D([theta], [r], marker=ms,
                                                    **eb_cap_style))

I noticed that the dynamic updates only happen when the ScaledRotation instance is passed directly to mmarkers.MarkerStyle. And the caps do not render dynamically when the adjusted_theta value is extracted from the class and then passed to mmarkers.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 after self._trans_shift.transform. but before the instance is passed to mmarkers.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:

class ScaledRotation(Affine2DBase):
    def __init__(self, theta: float, trans_shift: Transform, is_radial: bool) -> None: ...
    def get_matrix(self) -> np.ndarray: ...

An image comparison test:

@pytest.mark.parametrize("order", ["before", "after"])
@image_comparison(baseline_images=['polar_errorbar'], remove_text=True,
                  extensions=['png'], style='mpl20')
def test_polar_errorbar(order):
    theta = np.arange(0, 2 * np.pi, np.pi / 8)
    r = theta / np.pi / 2 + 0.5
    fig = plt.figure(figsize=(5, 5))
    ax = fig.add_subplot(projection='polar')
    if order == "before":
        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")
    else:
        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)

Examples 1:

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=(5, 5))
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("SW")
ax.set_theta_direction(-1)
ax.set_yticklabels([])
plt.show()
SW

Example 2:

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=(5, 5))
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("NW")
ax.set_theta_direction(1)
ax.set_yticklabels([])
plt.show()
NW

PR checklist

@kyracho kyracho marked this pull request as draft October 11, 2024 07:39
@QuLogic QuLogic added this to the v3.10.0 milestone Oct 12, 2024
@kyracho kyracho force-pushed the errorbar_caps_contribution branch 2 times, most recently from a12c70e to dd8a598 Compare October 13, 2024 12:58
@kyracho kyracho force-pushed the errorbar_caps_contribution branch from dd8a598 to 604cb9f Compare October 17, 2024 23:52
@kyracho kyracho force-pushed the errorbar_caps_contribution branch 2 times, most recently from 9d5202a to 912f347 Compare October 18, 2024 00:17
@kyracho kyracho force-pushed the errorbar_caps_contribution branch from 912f347 to 4abf666 Compare October 18, 2024 00:23
@kyracho
Copy link
Contributor Author

kyracho commented Oct 18, 2024

Hello, thanks so much for your feedback! I’ve incorporated the changes as suggested. Does everything look okay now?

@QuLogic
Copy link
Member

QuLogic commented Oct 18, 2024

Yes, is there a reason it's still draft?

@kyracho kyracho marked this pull request as ready for review October 18, 2024 01:27
@kyracho
Copy link
Contributor Author

kyracho commented Oct 18, 2024

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Member

@timhoffm timhoffm left a 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.

@ksunden ksunden merged commit bb6e1aa into matplotlib:main Oct 18, 2024
42 of 43 checks passed
@ksunden
Copy link
Member

ksunden commented Oct 18, 2024

Merged over that small comment because that assert is cheap and we are looking to close out 3.10 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Strange errorbar caps when polar axes have non-default theta direction or theta zero location
4 participants