Closed cvanelteren closed 1 year ago
Example:
Thanks for the contribution! Unfortunately, it looks like some tests are failing. I tried locally, and your update broke the logic. The issue is that the get_rotation
function is called whenever the axis size changes (e.g. when set_[xy]lim
is called). In your current implementation, it is only called once as far as I could tell.
You can test this locally with the following script:
import matplotlib.pyplot as plt
import numpy as np
from labellines import labelLine
fig = plt.figure()
x = np.linspace(0, 10, 1000)
y = np.sin(x)
line, = plt.plot(x, y, label="$\sin(x)$")
labelLine(line, 2*np.pi, align=True)
plt.show()
Screencast from 2023-09-27 13-58-00.webm
Note that you can also run tests locally with pytest:
# Create a virtual environment
python -m venv venv
source venv/bin/activate
python -m pip install '.[test]'
python -m pytest --mpl --mpl-results-path=results --mpl-generate-summary=html
Cool test setup! Didn't know pytest could export to html like this. The LabelLine
object inherents from Text
which has a default rotation implementation that should be compatible with this PR, what am I missing here? (see here for the implementation ).
EDIT: I am visually inspecting the failed tests but don't really see a difference by eye, e.g.
Maybe try having a look at test_rotation_correction
. From the test artefacts (https://github.com/cphyc/matplotlib-label-lines/actions/runs/6325489737?pr=161), I see this test as failing more obviously.
EDIT: I think your issue is that the rotation angle changes dynamically when the plot size changes. In your implementation _rotation
is set at initialisation and doesn't change anymore.
A bit more insight: your update breaks the piece of code that computes the rotation of the text to match the on-screen slope of the line. Since we're working in screen space, this depends on the size of the axis, which may have been updated since the LabelLines object got created.
So the _rotation
property needs to be recomputed and cannot be cached (when we're using the auto_align
keyword).
Ah I see! I pushed a fix with some comments. The tests pass now.
They do indeed! Let me review the PR now then.
@cphyc I too hastily pushed the local commits. My bad! Current state should be good with changes to core as well. The default behavior is still aligning the labels with the curve (which is preferred anyway). I just ran into this issue recently when I wanted the labels turned 90 degrees ;-)!
Thanks for cleaning up the mess @cphyc ;-)!
Hi,
Thanks for the nice package. I was missing the feature to manually control the angle of the label other than 0 degrees. This PR adds this feature as well as tidies up the code in
labelline.py
(i believe there is some unnecessary handling of properties going on). Sincerely, Casper