-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Improvements to plotting PMFs for discrete distributions #451
Conversation
I think it would be easier (and probably faster) to not use
|
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.
I guess it's the best that can be done currently. Maybe it would be a good opportunity to address #287 as well.
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Done! julia> plot(Binomial(50, 0.5)) |
This reverts commit 0ba4549.
Necessary to call UnitRange on them
I made the same work for discrete mixtures. e.g. here is zero-inflated Poisson: julia> zip = MixtureModel([Dirac(0), Poisson(10)], [0.1, 0.9])
MixtureModel{Distribution{Univariate, Discrete}}(K = 2)
components[1] (prior = 0.1000): Dirac{Int64}(value=0)
components[2] (prior = 0.9000): Poisson{Float64}(λ=10.0)
julia> plot(zip) julia> plot(zip; components=false) |
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.
Nice, thanks a lot!
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.
I wonder if one has to include markers for :sticks
by default?
src/distributions.jl
Outdated
end | ||
|
||
function default_range(m::Distributions.MixtureModel, alpha = 0.0001) | ||
minval = maxval = 0.0 | ||
minval = maxval = 0 |
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.
I guess this is more likely to be type unstable than the current implementation?
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.
Why? It turns out this is necessary for the zero-inflated Poisson to not make everything a Float64
. Since logpdf
for discrete MixtureModel
only accepts points of type Int
, this turning everything into floats causes failures.
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.
Because in the loop the type of minval
and maxval
will be changed for most distributions: for most distributions minimum
, maximum
and quantile
do not return values of type Int
. It's also not guaranteed that they are of type Float64
but this is definitely more likely for continuous distributions (and it would still be type stable for distributions where these values are of type Int
if one uses min
and max
to update minval
and maxval
in the loop).
logpdf
and pdf
accept Real
regardless if the distribution is discrete or not. If there's a problem with MixtureModel, it has to be fixed in Distributions.
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.
Because in the loop the type of
minval
andmaxval
will be changed for most distributions: for most distributionsminimum
,maximum
andquantile
do not return values of typeInt
. It's also not guaranteed that they are of typeFloat64
but this is definitely more likely for continuous distributions (and it would still be type stable for distributions where these values are of typeInt
if one usesmin
andmax
to updateminval
andmaxval
in the loop).
We should support both though, not just continuous distributions. I pushed an implementation using mapreduce
that is simpler and does this.
Co-authored-by: David Widmann <[email protected]>
This reverts commit 0f428de.
Can you clarify what you mean? |
If it is necessary to set |
Setting |
Yes, my question was if you think it would be preferrable to not show them explicitly. I prefer the version without markers (IMO it is a bit cleaner, in particular if there are many values to plot) and it seems simple enough to specify |
Okay, I simplified the implementation to not show markers by default. julia> plot(Binomial(50, 0.5)) For plots with few points, in my view this looks worse: julia> plot(plot(Binomial(5, 0.1)), plot(Binomial(5, 0.1); markershape=:circle)) But it looks much better for plots with many points: julia> plot(plot(Poisson(500)), plot(Poisson(500); markershape=:circle); legend=:left) |
This PR fixes #450 and fixes #287.