8000 FIX: Make stem() baseline follow the curvature in polar plots by timhoffm · Pull Request #29486 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FIX: Make stem() baseline follow the curvature in polar plots #29486

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2025
Merged

FIX: Make stem() baseline follow the curvature in polar plots #29486

merged 1 commit into from
Jan 25, 2025

Conversation

timhoffm
Copy link
Member

Closes #29484.

Note: There's precedence for a similar special-casing on polar Axes for errorbars:

matplotlib/lib/matplotlib/axes/_axes.py

Line 3798 in 3fb9c09

if self.name == "polar" and dep_axis == "x":

@timhoffm timhoffm added this to the v3.11.0 milestone Jan 20, 2025
@timhoffm timhoffm force-pushed the fix-polar-stem-baseline branch from c55c0ad to b3d768f Compare January 20, 2025 10:01
jklymak
jklymak reviewed Jan 20, 2025
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is 200 points enough? at 300 dpi (kind of usual publication quality) that would be a 1.7 cm circle which seems pretty small. Can we not use an Arc here?

(edit to correct 1.7 mm -> 1.7 cm)

@timhoffm
Copy link
Member Author

I’m not clear what you’ve calculated. Approximating a circle through a regular polygon with 200 nodes should not be visually distinguishable from a circle in all typical use cases. In fact, I believe 200 is more on the safe side.
It’s definitely enough in the example #29484 (comment).

This is a relatively cheap workaround. I’d rather not make the Artist type dependent on the Axes.

@jklymak
Copy link
Member
jklymak commented Jan 20, 2025

200 dots /(300 dots/inch) * 2.54 cm / inch=1.693 cm (I mistakenly typed mm instead of cm).

200 nodes should not be visually distinguishable from a circle in all typical use cases

That seems overly broad - if you don't want to do this "properly" (though I'm not sure that the type stability should be a concern here) then I'd at least use more points.

@timhoffm
Copy link
Member Author
timhoffm commented Jan 20, 2025 •
edited
Loading

Sorry, I’m still not clear. 200 are not some “dots” related to dpi, but the number of nodes in the approximating polygon. What you need to look at is the maximal deviation from the circle. See https://math.stackexchange.com/a/4132095. If I have calculated correctly from the top of my head n=200 results in D = 1e-4 R. So if you have a circle with R=1000px radius the maximal deviation is D=0.1px.

FWIW, I asked chatGPT for a recommendation on the number of nodes:

Practical Recommendations
• Small or medium circles viewed from a distance: 16-32 vertices.
• Close-up views or larger circles: Use 50-100 vertices.
• High precision applications (e.g., rendering, CAD): Over 100 vertices.

So, IMHO 200 is enough.

jklymak
jklymak approved these changes Jan 20, 2025
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

OK< fair enough - I agree, certainly compared to other imperfections this is almost impossible to detect.

@QuLogic
Copy link
Member
QuLogic commented Jan 21, 2025 •
edited
Loading

This should probably use the same intepolation method as axhline:

matplotlib/lib/matplotlib/axes/_axes.py

Line 805 in c2d502d

l.get_path()._interpolation_steps = mpl.axis.GRIDLINE_INTERPOLATION_STEPS

Or in fact, maybe it should use axhline itself?

@timhoffm timhoffm marked this pull request as draft January 22, 2025 01:00
@timhoffm timhoffm force-pushed the fix-polar-stem-baseline branch from b3d768f to 742fbe1 Compare January 22, 2025 02:05
@timhoffm
Copy link
Member Author

Thanks, it seems _interpolation_steps is the way to go. axhline itself cannot be used as it is in axes coordinates, but we need a line from the first to last data point, and hence data coordinates. hlines would give that, but that creates a LineCollection, which would be a bit heavy and an API break.

I've changed the test to "check that the baseline is reasonably interpolated, by verifying it has a large number of interpolation steps". This is a bit weak as it does only test the setting not the visual effect. OTOH I think it's not worth to add a test image and an image-comparison check that is less tautolgical is also hard (just manually drawing a line with _interpolation_steps set doesn't add more info). So IMHO the test is good enough.

@timhoffm timhoffm marked this pull request as ready for review January 22, 2025 02:09
QuLogic
QuLogic reviewed Jan 22, 2025
View reviewed changes
@timhoffm timhoffm force-pushed the fix-polar-stem-baseline branch from 742fbe1 to 05af7bf Compare January 22, 2025 08:10
@timhoffm timhoffm force-pushed the fix-polar-stem-baseline branch from 05af7bf to 0225617 Compare January 22, 2025 08:22
@scottshambaugh
Copy link
Contributor

Is there an image test that exercises this? I would expect that to need an update.

@timhoffm
Copy link
Member Author

There is no stem() test on a polar plot. Generally, we don’t test all functions with all projections.

@QuLogic QuLogic merged commit f39bf4d into matplotlib:main Jan 25, 2025
41 checks passed
@timhoffm timhoffm deleted the fix-polar-stem-baseline branch January 25, 2025 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.

[Bug]: On polar axis, stem plot with non-zero bottom has an extra red line.
4 participants
@timhoffm @jklymak @QuLogic @scottshambaugh
0