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

Allow weights in statistical recipes #1816

Merged
merged 5 commits into from May 3, 2022
Merged

Conversation

greimel
Copy link
Contributor

@greimel greimel commented Apr 12, 2022

Add the keyword weights to boxplot, density, hist, violin.

density

image

let
	N = 100_000
	x = rand(Uniform(-2, 2), N)
	
	w = pdf.(Normal(), x)

	fig = Figure()
	density(fig[1,1], x)
	density(fig[1,2], x, weights = w)

	fig
end

violin

image

let
	N = 100_000
	x = rand(1:3, N)
	y = rand(Uniform(-1, 5), N)
	
	w = pdf.(Normal(), x .- y)

	fig = Figure()

	violin(fig[1,1], x, y)
	violin(fig[1,2], x, y, weights = w)

	fig
end

boxplot

image

let
	N = 100_000
	x = rand(1:3, N)
	y = rand(Uniform(-1, 5), N)
	
	w = pdf.(Normal(), x .- y)

	fig = Figure()

	boxplot(fig[1,1], x, y)
	boxplot(fig[1,2], x, y, weights = w)

	fig
end

hist

image

let
	N = 100_000
	x = rand(Uniform(-5, 5), N)
	
	w = pdf.(Normal(), x)

	fig = Figure()
	hist(fig[1,1], x)
	hist(fig[1,2], x, weights = w)

	fig
end

@greimel
Copy link
Contributor Author

greimel commented Apr 19, 2022

Friendly bump 😃

The added functionality generalizes what AlgebraOfGraphics.density() and AlgebraOfGraphics.histogram() can already do. (e.g. data(df) * mapping(:y, weights = :wgts) * density() |> draw)

@piever, since you have written most of the code that this PR touches – would you mind reviewing it if you find some time?

src/stats/violin.jl Outdated Show resolved Hide resolved
@@ -71,6 +73,7 @@ function plot!(plot::Violin)
npoints = n,
(bound === automatic ? NamedTuple() : (boundary = bound,))...,
(bw === automatic ? NamedTuple() : (bandwidth = bw,))...,
(w === automatic ? NamedTuple() : (weights = StatsBase.weights(view(w, idxs)),))...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a utility function to filter out automatic keywords, but that should be done separately.

src/stats/boxplot.jl Outdated Show resolved Hide resolved
@piever
Copy link
Contributor

piever commented Apr 20, 2022

Nice! Looks good to me (just left two tiny formatting comments).

I have a minor doubt as to whether we should call StatsBase.weights indiscriminately, or instead not call it when the user passes a StatsBase.AbstractWeight object. I have no idea whether the various subtypes of AbstractWeight make any difference here. I suspect not, so probably it's fine to leave as is.

Co-authored-by: Pietro Vertechi <pietro.vertechi@protonmail.com>
@greimel
Copy link
Contributor Author

greimel commented Apr 22, 2022

I've applied your suggestions, thanks for the review!

@greimel greimel requested a review from piever April 22, 2022 08:29
@SimonDanisch
Copy link
Member

Great!
There is still an error in the docs:

┌ Franklin Warning: in <examples/plotting_functions/violin.md>
│ There was an error of type 'ArgumentError' when running a code block.
│ Checking the output files '/home/runner/work/Makie.jl/Makie.jl/docs/__site/assets/examples/plotting_functions/violin/code/output/example_figure.(out|res)'
│ might be helpful to understand and solve the issue.
│ 
│ Relevant pointers:
│ - evaluation of Julia code blocks: https://franklinjl.org/code/
│ 
│ Details:
│ ArgumentError: Package Distributions not found in current path:
│ - Run `import Pkg; Pkg.add("Distributions")` to install the Distributions package.

Can you add that to the docs project?

@greimel
Copy link
Contributor Author

greimel commented Apr 28, 2022

Done! I hadn't realized that the docs are actually built for non-member contributors these days. That's great news!

@greimel
Copy link
Contributor Author

greimel commented May 1, 2022

I would like to use this for a lecture in about a week.

If it's not too much of a hassle for you, I'd appreciate this being merged and tagged before May 8.

If you don't plan a new tag soon, please let me know and I'll adjust my material accordingly

@SimonDanisch SimonDanisch merged commit c473142 into MakieOrg:master May 3, 2022
@greimel greimel deleted the weights branch May 3, 2022 08:48
@SimonDanisch
Copy link
Member

I'll try to release a new version!

@SimonDanisch
Copy link
Member

Would a breaking version work for you as well?

@greimel
Copy link
Contributor Author

greimel commented May 3, 2022

that's fine, thanks for asking!

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.

None yet

3 participants