Skip to content
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

able to scale violin in different ways #3352

Closed
wants to merge 7 commits into from
Closed

Conversation

ctarn
Copy link
Contributor

@ctarn ctarn commented Nov 2, 2023

fix #3351

@ctarn

This comment was marked as outdated.

@ctarn
Copy link
Contributor Author

ctarn commented Nov 3, 2023

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 ctarn marked this pull request as draft November 3, 2023 10:55
@ctarn
Copy link
Contributor Author

ctarn commented Nov 3, 2023

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

@ctarn
Copy link
Contributor Author

ctarn commented Nov 4, 2023

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 ctarn marked this pull request as ready for review November 4, 2023 12:57
@ctarn ctarn changed the title scale violin area by amount able to scale violin in different ways Nov 4, 2023
@jkrumbiegel jkrumbiegel reopened this Feb 22, 2024
@jkrumbiegel
Copy link
Member

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.

@ctarn
Copy link
Contributor Author

ctarn commented Feb 22, 2024

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

@ctarn
Copy link
Contributor Author

ctarn commented Feb 25, 2024

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

@jkrumbiegel
Copy link
Member

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.

@ctarn
Copy link
Contributor Author

ctarn commented Feb 25, 2024

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?

@jkrumbiegel
Copy link
Member

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

@ctarn
Copy link
Contributor Author

ctarn commented Feb 26, 2024

@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]

@jkrumbiegel
Copy link
Member

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

@ctarn
Copy link
Contributor Author

ctarn commented Feb 26, 2024

:). 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.

@jkrumbiegel
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sizes of areas in violin plot are incorrect
2 participants