Use Path.arc() to interpolate polar arcs. by anntzer · Pull Request #14852 · matplotlib/matplotlib · GitHub

Currently, in polar plots, arcs are rendered by linearly interpolating
in (theta, r) space before conversion to (x, y) space (if their
_interpolation_steps attribute is >1 -- this attribute is described in
the docs as "an implementation detail not intended for public use").

When _interpolation_steps == 1, this PR doesn't change anything -- there
is an example (specialty_plots/radar_chart) that explicitly relies on
being able to set _interpolation_steps to 1 and get a "polygonal" polar
axes rather than a circular one.

When _interpolation_steps > 1, however, this PR looks for paths segments
that are either at a constant theta or a constant r. It transforms
constant-theta segments to a single segment (because interpolation
doesn't help there) and transforms constant-r segments (circular arcs)
to a Bezier approximation of a circle which is already implemented in
Path.arc()).

This greatly decreases the number of control points for such plots in
vector outputs (which are thus smaller -- note the line counts changes
in the svg files...), and also improves rasterization by mplcairo (cairo
appears to be not so good at filling areas with a lot of close vertices).

The many changes in baseline images are due to, well, the change of
approximation for drawing circles. (Note that if we agree with the
approach of this PR, I could perhaps do a first PR to decrease the
number of polar tests so that I don't have to commit so many new
images...)

Example: polar(); bar([0], [0.9])

matplotlib svg before, showing control points in inkscape:

matplotlib svg after, showing control points in inkscape:

mplcairo before:

mplcairo after:

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

This looks really useful. Can the old images be kept by setting interpolation_steps=1? OTOH I think if a default changes we should consider buying the bullet and changing the test images. Same for the image antialiasing PR

No, interpolation_steps=1 results in something like https://matplotlib.org/3.1.0/gallery/specialty_plots/radar_chart.html.

I'm fine with modifying a few baseline images but I think it's also a good opportunity to see whether we can combine some tests...

Well, I like the idea, but I won't approve since you want to work on thinning out the test images. Also, should try and get these new bits below to be tested.

This block seems untested.

test added, although it's not particularly interesting (I can't actually think of a very interesting test for that...)

As does this block.

same as above

And this block too.

test added

Is this really better than codes.extend([Path.LINETO] * len(trs))?

either way works for me, switched to your preference.

Rebased on top of #14888 which deletes a few baseline images (replacing them by check_figures_equal).

Also, note that with this patch, one cannot anymore set e.g. GRIDLINE_INTERPOLATION_STEPS to a small but >1 number (e.g. 3) and see 3-point interpolation on circular arcs, but I'm not too worried about that...

I'm positive on the idea of this change. Ping when you think it's ready for review.

This PR includes #14888 so it can already be reviewed (at worst this one gets merged before #14888 and both commits go in at the same time). (I don't think the other polar baseline images can "easily" be removed.)

Docs fail, might need a rebase.

rebased

Currently, in polar plots, arcs are rendered by linearly interpolating in (theta, r) space before conversion to (x, y) space (if their _interpolation_steps attribute is >1 -- this attribute is described in the docs as "an implementation detail not intended for public use"). When _interpolation_steps == 1, this PR doesn't change anything -- there is an example (specialty_plots/radar_chart) that explicitly relies on being able to set _interpolation_steps to 1 and get a "polygonal" polar axes rather than a circular one. When _interpolation_steps > 1, however, this PR looks for paths segments that are either at a constant theta or a constant r. It transforms constant-theta segments to a single segment (because interpolation doesn't help there) and transforms constant-r segments (circular arcs) to a Bezier approximation of a circle which is already implemented in Path.arc()). This greatly decreases the number of control points for such plots in vector outputs (which are thus smaller), and also improves rasterization by mplcairo (cairo appears to be not so good at filling areas with a lot of close vertices). The many changes in baseline images are due to, well, the change of approximation for drawing circles.

Bit unfortunate about all the test images, I guess.

I don't mind waiting to see at least whether the test-images-infrastructure-rewrite GSoC will happen or not (after all this has already been there for 8 months, an extra couple of them isn't too bad...).

Decided to merge as-is as this is a pretty big improvement for SVG and mplcairo.

anntzer added the topic: polar label Jul 18, 2019

QuLogic reviewed Jul 25, 2019

View reviewed changes

anntzer mentioned this pull request Jul 25, 2019

Replace some polar baseline images by check_figures_equal. #14888

Merged

6 tasks

anntzer force-pushed the polarinterp branch from 71cf4b5 to 89ffe8c Compare July 25, 2019 13:34

anntzer force-pushed the polarinterp branch from 89ffe8c to e956ad1 Compare July 25, 2019 13:57

jklymak requested a review from QuLogic July 26, 2019 23:15

anntzer force-pushed the polarinterp branch 2 times, most recently from 2701391 to 3e74404 Compare July 28, 2019 21:45

tacaswell added this to the v3.3.0 milestone Aug 12, 2019

anntzer force-pushed the polarinterp branch 2 times, most recently from 0842a13 to 851d81a Compare August 12, 2019 21:25

anntzer force-pushed the polarinterp branch from 851d81a to 31133ea Compare September 8, 2019 08:39

QuLogic added the status: needs rebase label Mar 7, 2020

anntzer force-pushed the polarinterp branch from 31133ea to 73a763a Compare March 9, 2020 11:08

anntzer removed the status: needs rebase label Mar 9, 2020

QuLogic approved these changes Mar 13, 2020

View reviewed changes

tacaswell merged commit 4331c53 into matplotlib:master Mar 13, 2020

anntzer deleted the polarinterp branch March 13, 2020 20:35

anntzer mentioned this pull request May 15, 2020

[feature request] circle -> SVG <circle> #17424

Open

anntzer mentioned this pull request Apr 16, 2021

Radar Chart with Polygonal Grid Lines #19981

Closed

This was referenced Aug 21, 2022

Polar errcaps #23592

Merged

Document polar handling of _interpolation_steps. #23695

Merged

+                                           * self._axis.get_rsign())
+                                  if last_td <= td:
+                                      while td - last_td > 360:
+                                          arc = Path.arc(last_td, last_td + 360)

+                                      # The reverse version also relies on the fact that all
+                                      # codes but the first one are the same.
+                                      while last_td - td > 360:
+                                          arc = Path.arc(last_td - 360, last_td)

+                                      xys.extend(arc.vertices[::-1][1:] * r)
+                                      codes.extend(arc.codes[1:])
+                              else:  # Interpolate.
+                                  trs = cbook.simple_linear_interpolation(

+                                      np.column_stack([(last_t, last_r), trs]),
+                                      path._interpolation_steps)[1:]
+                                  xys.extend(self.transform_non_affine(trs))
+                                  codes.extend(Path.LINETO for _ in trs)

Use Path.arc() to interpolate polar arcs. by anntzer · Pull Request #14852 · matplotlib/matplotlib · GitHub

Repository navigation

Use Path.arc() to interpolate polar arcs. - #14852

Use Path.arc() to interpolate polar arcs.#14852tacaswell merged 1 commit intomatplotlib:mastermatplotlib/matplotlib:masterfrom anntzer:polarinterpanntzer/matplotlib:polarinterpCopy head branch name to clipboard

Conversation

anntzer commented Jul 18, 2019

PR Summary

PR Checklist

Uh oh!

jklymak commented Jul 18, 2019

Uh oh!

anntzer commented Jul 18, 2019

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Jul 25, 2019

Uh oh!

anntzer commented Jul 25, 2019

Uh oh!

timhoffm commented Jul 27, 2019

Uh oh!

anntzer commented Jul 27, 2019

Uh oh!

QuLogic commented Sep 8, 2019

Uh oh!

anntzer commented Sep 8, 2019

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Mar 13, 2020

Uh oh!

tacaswell commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Use Path.arc() to interpolate polar arcs.#14852
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:polarinterp