Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use Path.arc() to interpolate polar arcs.#14852
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 likehttps://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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| * self._axis.get_rsign()) | ||
| if last_td <= td: | ||
| while td - last_td > 360: | ||
| arc = Path.arc(last_td, last_td + 360) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This block seems untested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
test added, although it's not particularly interesting (I can't actually think of a very interesting test for that...)
| # 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As does this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above
| xys.extend(arc.vertices[::-1][1:] * r) | ||
| codes.extend(arc.codes[1:]) | ||
| else: # Interpolate. | ||
| trs = cbook.simple_linear_interpolation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And this block too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
test added
lib/matplotlib/projections/polar.py Outdated
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this really better thancodes.extend([Path.LINETO] * len(trs))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
2701391 to3e74404Compare0842a13 to851d81aCompareDocs fail, might need a rebase. |
rebased |
Currently, in polar plots, arcs are rendered by linearly interpolatingin (theta, r) space before conversion to (x, y) space (if their_interpolation_steps attribute is >1 -- this attribute is described inthe docs as "an implementation detail not intended for public use").When _interpolation_steps == 1, this PR doesn't change anything -- thereis an example (specialty_plots/radar_chart) that explicitly relies onbeing able to set _interpolation_steps to 1 and get a "polygonal" polaraxes rather than a circular one.When _interpolation_steps > 1, however, this PR looks for paths segmentsthat are either at a constant theta or a constant r. It transformsconstant-theta segments to a single segment (because interpolationdoesn't help there) and transforms constant-r segments (circular arcs)to a Bezier approximation of a circle which is already implemented inPath.arc()).This greatly decreases the number of control points for such plots invector outputs (which are thus smaller), and also improves rasterizationby mplcairo (cairo appears to be not so good at filling areas with a lotof close vertices).The many changes in baseline images are due to, well, the change ofapproximation for drawing circles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. |
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