Replace radial_distortion_threshhold with directly setting radius_at_origin #3381
Conversation
Compile Times benchmarkNote, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running: using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(fig)
|
Personally I think:
@mikeingold @anowacki @jkrumbiegel @SimonDanisch Maybe any of you have some more thoughts on this? |
|
Wow! Thanks for the swift response and PR.
|
|
We had quite a bit of discussion in #3154 about having a positive r shift as a default and decided not to. You're right that the placement is still correct, but having shapes distort may mislead people when they're not aware of it. Having users explicitly adjust some attribute can be a useful barrier here, I think. From that point of view we should have |
|
It sounds like the core principles we'd like to respect are:
In the case of strictly non-negative data, both principles are satisfied by just pinning In the presence of negative data, though, we can't simultaneously satisfy both principles by default. So then the question in my mind is: which do most users expect should take precedence? In my experience, polar plots with negative values are very common, and the data isn't expected to have a literal physical interpretation. As a mostly-casual Makie user, my experience with this was roughly: plot with a default tl;dr, In my opinion:
|
| function data_limits(p::Triplot{<:Tuple{<:Vector{<:Point}}}) | ||
| if transform_func(p) isa Polar | ||
| # Because the Polar transform is handled explicitly we cannot rely | ||
| # on the default data_limits. (data limits are pre transform) | ||
| iter = (to_ndim(Point3f, p, 0f0) for p in p.converted[1][]) | ||
| limits_from_transformed_points(iter) | ||
| else | ||
| # First component is either another Voronoiplot or a poly plot. Both | ||
| # cases span the full limits of the plot | ||
| data_limits(p.plots[1]) | ||
| end | ||
| end |
There was a problem hiding this comment.
Test failures come from this adjusting PolarAxis limits a little. It's the same code as we already have for voronoiplots
I've never used polar plots for real (not common at all in neuroscience / psychology) so I don't have much of an opinion. But this line of reasoning sounds valid to me, and we could adjust r to include all data in case there's negative data. How are other plotting libraries handling this? |
Description
Fixes #3375 by allowing users to set a radial offset that can be used to map a negative rmin to 0. E.g.:
Some open questions:
radius_at_origin?theta_0to work like r0/radius_at_origin? Currently we havetheta_out = theta_in + theta_0andr_out = r_in - r0Note that offsetting radii results in distortions:
Changes
radial_distortion_thresholdwithradius_at_originwhich allows you to set an offset for radiiclip_r::Boolto PolarAxis and Polar transform, allowing you to set whether to enforceclip_r = true(old behavior) returnNaNrather thanr = 0ifType of change
Checklist