-
-
Notifications
You must be signed in to change notification settings - Fork 313
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PolarAxis improvements (full rlimits, thetalimits, improved controls, argument order) #3154
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(display(fig))
|
If it's not too bold to comment, I personally have no problem with a 'slim ring' when I have asked for a small range of r, so I would be happy with that. (I don't have any opinion on the implementation as I don't understand that.) |
I think it would be better to not go the "slim ring" route and instead do it via shift of the transform (a slimmer ring might still be desirable to keep the amount of radial stretching or "aspect ratio" of the data at some desired value. That would make the transform not fully invertible, but I guess if we throw a clear error if any data clips inwards that's ok? |
Yea I added an
Since we don't use the inverse transform in PolarAxis at all we could, I guess, but I'm not sure if it's good to do that. It's quite easy for stuff to end up with r < 0 through zooming (and now translating) the axis. We probably shouldn't throw an error every time that happens if someone is using the inverse transform in some way. Probably better to just hide/leave stuff at r = 0. |
Missing reference imagesFound 1 new images without existing references. |
Some Notes on how I currently have things set up:
Updated test plot: f = Figure()
theta_0 = Slider(f[1, 1], range = range(0, 2pi, length=101)).value
thetalims = IntervalSlider(f[2, 1], range = range(0, 2pi, length=101)).interval
rlims = IntervalSlider(f[3, 1], range = range(0, 11, length=111)).interval
po = PolarAxis(f[4, 1:2], thetalimits = thetalims, rlimits=rlims, theta_0 = theta_0)
po.theta_translation_button[] = Mouse.right
bb = map(Makie.transformed_bbox,
po.target_radius, po.thetalimits,
map(x -> x.r0, po.scene.transformation.transform_func),
po.direction,
po.theta_0,
)
# these are the real values, not the values set by the slider (axis interaction causes desync)
Label(f[1, 2], map(x -> @sprintf("θ₀ = %0.1f°", x * 180/pi), po.theta_0))
Label(f[2, 2], map(lims -> @sprintf("θ = %0.1f° .. %0.1f°", (180/pi .* lims)...), po.thetalimits))
Label(f[3, 2], map(lims -> @sprintf("r = %0.2f .. %0.2f", lims...), po.target_radius))
p = scatter!(po, Point2f.(2, 2pi .* (0:7) ./ 8), color = 1:8, colormap = :RdBu)
lines!(po, range(1, 10, 101), range(0, 2pi, length=101))
p = scatter!(po, Point2f.(10, 2pi .* (0:7) ./ 8), color = 1:8, colormap = :RdBu)
l = lines!(po, bb, color = :yellow, transformation = Transformation())
translate!(l, 0, 0, 9000)
f |
Missing reference imagesFound 1 new images without existing references. |
Missing reference imagesFound 1 new images without existing references. |
Ok, the things you noted should be taken care of now. Since I switched back to full-circle PolarAxis by default I also adjusted the default interactions:
Other changes:
|
Missing reference imagesFound 1 new images without existing references. |
src/makielayout/blocks/polaraxis.jl
Outdated
function Makie.plot!( | ||
po::PolarAxis, P::Makie.PlotFunc, | ||
attributes::Makie.Attributes, args...; | ||
needs_tight_limits(::Surface) = true |
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.
One set of test failures (basically everything surface related) comes from this, because this also affects Axis
. Personally I think surface plots on an Axis defaulting to tight limits is something we want but I can also just revert this.
The other test failures around PolarAxis have a few sources... PolarAxis Decorations before:After:
PolarAxis scatterlines spine before:After:This has a different default rtick angle, which is also coming from rtick alignment in polar sectors and is easy to revert if we want. It also moved away from tight limits, which I think we want? Voronoiplot with a nonlinear transform, Triplot with nonlinear transformationSame rtick angle change as above tl;dr question @jkrumbiegel, @SimonDanisch
|
I'd say we shouldn't!
I like them on a straight line but don't have a strong opinion ;) |
The straight line would be problematic with slightly longer ticks, I'd argue for keeping them horizontal because of that. |
Looking back at this I think there are two specific question about rticks:
I think we should, because in a 2d Axis surface is more or less the same as image which has tight limits. But if we don't I'd just surface untightened here too |
I meant, we shouldn't do it in this PR, since its unrelated and strictly speaking breaking (by the definition that we need to update reference images). |
Removed/Commented out the surface tightlimits change, so now a surface plot in PolarAxis produces (from test w/o rlims!() call) Default rtickangle is unchanged, so the default rtick placement is still on a straight horizontal line: With explicitly set rtickangle, the individual label rotation is now horizontal if thetalimits are > 1.9pi and ":aligned" otherwise. That means they rotate according to their position, but stay close to upright and horizontal |
@ffreyer and I decided to merge this to get it in the next release, since there are only styling changes pending and no other breaking change in sight. |
Changelog
Breaking changes
(r, theta)
to(theta, r)
. This can be changed back withpolaraxis.theta_as_x[] = false
ax.radius
representing rmax toax.rlimits
representing (rmin, rmax)`Visual/Interactivity changes
ax.rzoomkey = Keyboard.r
andax.thetazoomkey = Keyboard.theta
and more permanently withax.rzoomlock = false
,ax.thetazoomlock = true
andax.fixrmin = true
(defaults)polaraxis.griddepth[] = 8999
.New Additions/Features
delete!(::PolarAxis, ::Plot)
rlims!(ax, rmin, rmax)
thetalims!(ax, thetamin, thetamax)
andax.thetalimits[]
autolimits!(ax[, unlock = true])
to allow the PolarAxis to derive its limits freely and unlock interactions.rticklabelrotation = automatic
to orient rtick labels according to the corresponding grid lines.zoomspeed = 0.1
to control the speed of zoomingrzoomkey = Keyboard.r
to restrict zooming to the r directionthetazoomkey = Keyboard.t
to restrict zooming to the theta directionfixrmin = true
to control whether zooming and translating affects rminrzoomlock = false
andthetazoomlock = true
to block zooming in the respective directionsr_translation_button = Mouse.right
for translating the plot in theta directiontheta_translation_button = Mouse.right
for translating the axis in theta directionaxis_rotation_button = Keyboard.left_control & Mouse.right
to rotate the axis as a wholereset_axis_orientation = false
to control whether the reset interaction reorients the axisnormalize_theta_ticks = true
to control whether theta ticks are normalized to a -2pi..2pi rangeBackground Changes
sample_density
now applies to clip polygonstarget_thetalims
AngularTicks
to derive more pleasant theta ticksDescription
PolarAxis
: Allow limits on radius and angle #3152, adding rlimits and thetalimits to generate circle sectorsax.griddepth
and adjusting the default behaviourPolarAxis
(and make the swapped version default) #3178 by addingtheta_as_x
and changing the default argument orderTODO
improveaddMultipleTicks
for anglesAngularTicks
rlims!
, addthetalims!
detach clip orientation from theta_0 and theta limits. You should be able to adjustReview
maximum_clip_radius
(is it better?)direction = -1
rtick_rotation = true/false/automatic/angle
attributereconsiderdirection * (theta + theta_0)
vsdirection * theta + theta_0
isfinite(vmax)
error when translating a lotType of change
Checklist