Makie.jl icon indicating copy to clipboard operation
Makie.jl copied to clipboard

able to scale violin in different ways

Open ctarn opened this issue 1 year ago • 4 comments

fix #3351

ctarn avatar Nov 02 '23 14:11 ctarn

tested with:

using CairoMakie
xs = rand([1, 2, 2, 3, 3, 3], 10000)
println([sum(xs .== x) for x in 1:3])
ys = randn(10000)
violin(xs, ys; show_median=true)

display

ctarn avatar Nov 02 '23 14:11 ctarn

I would like to convert it into a draft since I noticed that scaling by amount should not be treated as a default behavior, and matplotlib doesn't scale it either. I would like to suggest adding it as an optional behavior by adding a keyword parameter scale.

ctarn avatar Nov 03 '23 10:11 ctarn

seaborn supports scaling by amount by setting density_norm="count": https://seaborn.pydata.org/generated/seaborn.violinplot.html

ctarn avatar Nov 03 '23 11:11 ctarn

now violin supports a new parameter scale, which can be set to :area, :count, and :width. it is set to :area by default to fit the original behavior, and thus it would not be a breaking change.

using Makie, CairoMakie

fig = Figure()
xs = vcat([fill(i, i * 1000) for i in 1:4]...)
ys = vcat(randn(6000), randn(4000) * 2)
for (i, scale) in enumerate([:area, :count, :width])
    ax = Axis(fig[i, 1])
    violin!(ax, xs, ys; scale, show_median=true)
    Makie.xlims!(0.2, 4.8)
    ax.title = "scale=:$(scale)"
end
fig

plot_29

ctarn avatar Nov 04 '23 12:11 ctarn

Sorry for the delay, this looks good and useful to me! Only needs a changelog entry, and ideally a reference test in here https://github.com/MakieOrg/Makie.jl/blob/master/ReferenceTests/src/tests/examples2d.jl if you could add one. I don't seem to be able to edit your branch.

jkrumbiegel avatar Feb 22 '24 08:02 jkrumbiegel

Thank you very much! I will update it soon :)

ctarn avatar Feb 22 '24 08:02 ctarn

Hi. Sorry. I am not familiar with reference test. Would it be ok if I add a test case just like the one added to docs/reference/plots/violin.md?

fig = Figure()
xs = vcat([fill(i, i * 1000) for i in 1:4]...)
ys = vcat(randn(6000), randn(4000) * 2)
for (i, scale) in enumerate([:area, :count, :width])
    ax = Axis(fig[i, 1])
    violin!(ax, xs, ys; scale, show_median=true)
    Makie.xlims!(0.2, 4.8)
    ax.title = "scale=:$(scale)"
end
fig

ctarn avatar Feb 25 '24 09:02 ctarn

Yes exactly, the reference test is just supposed to confirm that the visual output of a plotting function stays the same over time, so you can reuse the example that demonstrates the functionality in the docs.

jkrumbiegel avatar Feb 25 '24 11:02 jkrumbiegel

The changelog and reference test have been added. The checks report 3 missing reference images must be uploaded (I only added one test). I didn't find docs about uploading such images. Is there anything I can do further?

ctarn avatar Feb 25 '24 13:02 ctarn

All good, we have to upload those. Three because of the three tested backends.

jkrumbiegel avatar Feb 25 '24 17:02 jkrumbiegel

@jkrumbiegel Hi! I run into the following error when click Update reference images with selection in my browser. It seems that I don't have access to create a release?

julia> ReferenceUpdater.serve_update_page(pr=3352)
[ Info: PR is 3352, using last commit hash 54efcc359f4bb9522f9228b442ece3a2044e104b
[ Info: Choosing artifact "ReferenceImages"
[ Info: Downloading artifact from https://api.github.com/repos/MakieOrg/Makie.jl/actions/artifacts/1272957341/zip
[ Info: Download successful
[ Info: Extracting zip file /var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_Dy3Y3sjqjO to /var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_mFO3TT
[ Info: Extracted zip file
[ Info: Serving update page from folder /var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_mFO3TT.
[ Info: Starting server. Open http://localhost:8849 in your browser to view. Ctrl+C to quit.
[ Info: Listening on: 127.0.0.1:8849, thread id: 1
[ Info: Copying reference folder to "/var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_vM96xUNyPW"
[ Info: Overwriting "WGLMakie/Violin plots differently scaled.png" in new reference folder
[ Info: Overwriting "CairoMakie/Violin plots differently scaled.png" in new reference folder
[ Info: Overwriting "GLMakie/Violin plots differently scaled.png" in new reference folder
[ Info: Uploading updated reference images under tag "v0.20.0"
WARNING: found release (v0.20.0). Use existing one.
--> Deleting: reference_images.tar
Failed to delete existing assets: one of the goroutines failed: failed to delete asset: reference_images.tar: failed to delete release asset: DELETE https://api.github.com/repos/MakieOrg/Makie.jl/releases/assets/152819916: 403 Resource not accessible by personal access token []
failed process: Process(`/Users/i/.julia/artifacts/1bc0af717efc4628a0863729a08c53c7b1c0c184/bin/ghr -replace -u MakieOrg -r Makie.jl -t {{seems to be my github token}} v0.20.0 /var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_BFbfwl/reference_images.tar`, ProcessExited(11)) [11]

v0.20.0 is the default tag. When I try other tags:

[ Info: Uploading updated reference images under tag "v0.20.9"
==> Create a new release
Failed to create GitHub release page: failed to create a release: POST https://api.github.com/repos/MakieOrg/Makie.jl/releases: 403 Resource not accessible by personal access token []
failed process: Process(`/Users/i/.julia/artifacts/1bc0af717efc4628a0863729a08c53c7b1c0c184/bin/ghr -replace -u MakieOrg -r Makie.jl -t {{seems to be my github token}} v0.20.9 /var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_XoFNKA/reference_images.tar`, ProcessExited(11)) [11]
[ Info: Uploading updated reference images under tag "v0.20.9-violin"
==> Create a new release
Failed to create GitHub release page: failed to create a release: POST https://api.github.com/repos/MakieOrg/Makie.jl/releases: 403 Resource not accessible by personal access token []
failed process: Process(`/Users/i/.julia/artifacts/1bc0af717efc4628a0863729a08c53c7b1c0c184/bin/ghr -replace -u MakieOrg -r Makie.jl -t {{seems to be my github token}} v0.20.9-violin /var/folders/r3/y1sclb514fxbrwhgpk1vk2x00000gn/T/jl_85KlP9/reference_images.tar`, ProcessExited(11)) [11]

ctarn avatar Feb 26 '24 09:02 ctarn

Sorry I wasn't clear, by we have to upload those I meant we maintainers with the correct access rights :) Nothing you need to do right now

jkrumbiegel avatar Feb 26 '24 09:02 jkrumbiegel

:). Thanks.

BTW, there is an error in the readme of ReferenceUpdater. The package should be installed using

Pkg.develop(path=joinpath(dirname(dirname(pathof(Makie))), "ReferenceUpdater"))

The original cmd will run into errors.

ctarn avatar Feb 26 '24 09:02 ctarn

thanks for the PR! It's merged from the other branch now where I had to make some small edits :)

jkrumbiegel avatar Mar 19 '24 08:03 jkrumbiegel