-
Notifications
You must be signed in to change notification settings - Fork 432
Implement Dirac distribution #861
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
Conversation
|
Hi @aplavin have you checked: f4a3e5c |
|
I haven't seen that commit specifically, but I guessed that all distributions should correctly handle edge cases correctly. E.g. sd = 0 for Normal, Cauchy, TDist and others, or lower = upper for Uniform, should all give a degenerate Dirac-delta distribution. I see two obvious advantages to having a special distribution like this: it should be more performant - basically no computations at all; and one shouldn't have to choose which distribution edge case he wants, so that there is always e.g. |
|
|
|
So, I will rename |
|
Looks good to me. Add all necessary functions, some tests, documentation. Then we will see how to use it for degenerate cases of distributions, ie: |
|
Falling back to |
|
Good point. @aplavin you can finish this implementation without touching the special
|
|
ping @aplavin how are you doing on this one? Do you need help on some parts? |
src/univariate/discrete/dirac.jl
Outdated
|
|
||
| eltype(::Dirac{T}) where {T} = T | ||
| rand(rng::AbstractRNG, d::Dirac) = d.value | ||
| pdf(d::Dirac, x::Real) = x == d.value ? 1 : 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.
these should be floating point
| pdf(d::Dirac, x::Real) = x == d.value ? 1 : 0 | |
| pdf(d::Dirac, x::Real) = x == d.value ? 1.0 : 0.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.
Shouldn't pdf(d, NaN) return NaN? Or what if d.value is NaN?
src/univariate/discrete/dirac.jl
Outdated
| eltype(::Dirac{T}) where {T} = T | ||
| rand(rng::AbstractRNG, d::Dirac) = d.value | ||
| pdf(d::Dirac, x::Real) = x == d.value ? 1 : 0 | ||
| cdf(d::Dirac, x::Real) = x < d.value ? 0 : 1 |
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.
| cdf(d::Dirac, x::Real) = x < d.value ? 0 : 1 | |
| cdf(d::Dirac, x::Real) = x < d.value ? 0.0 : 1.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.
same everywhere
src/univariate/discrete/dirac.jl
Outdated
| A *Dirac distribution* is parametrized by its only value, and takes its value with probability 1. | ||
| ```julia | ||
| d = Dirac(2.) # Dirac distribution with value = 2. |
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'd use 2.0 rather than 2..
Also, punctuation in comments.
src/univariate/discrete/dirac.jl
Outdated
| ```julia | ||
| d = Dirac(2.) # Dirac distribution with value = 2. | ||
| rand(d) # Always returns the same value |
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.
Again, punctuation, and the comment isn't aligned with the one above (which I assume is what you wanted).
|
ping @aplavin |
|
@aplavin the build is still failing on travis |
| rand(d) # Always returns the same value | ||
| ``` | ||
| """ | ||
| struct Dirac{T} <: DiscreteUnivariateDistribution |
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.
Dirac Delta is a Continuous distribution
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.
No, it doesn't have a density with respect to the Lebesgue measure, so it is not a "continuous probability distribution" (not to mix with distributions in Schwartz' sense.)
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.
According to the documentation ValueSupport can take values Discrete (Samples take discrete Int values) and Continuous (samples take continuous real Float64 values). So in this sense, it seems that the Dirac delta should be Continuous. I think that your interpretation would leave many useful measures completely out of the hierarchy (e.g. mixtures of continuous and discrete such as Spike-and-Slab)
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.
That is, according to the documentation the trait "Continuous" seems to refer only to the set on which the distribution is defined (the real line), not to the fact that it is absolutely continuous with respect to the Lebesgue measure.
|
What is the status of this PR? It would be very useful to have a |
|
I think technically you cannot update. This is quite progressed, I think we need to check and test some stupid things about NaNs and review the properties below with the literature |
|
But if you want to work on this by making a PR starting from this I think this is okay with @aplavin ? (You can also make PRs against this branch) |
|
Yes I confirm this is totally ok with me. Sorry for abandoning the PR, at that time I was completely unaware of how all the git/branch/pr/pkg interact together and couldn't even reproduce the tests locally. The CI gave a confusing error with unhelpful stacktrace and I just couldn't debug it. Now I kinda-understand how everything works together, but still not quite... |
|
I think |
|
In my opinion, it depends on what distribution you have in mind when talking about a Dirac distribution. For the use case in Turing that popped up yesterday we are interested in the discrete measure |
I don't think that the name Dirac should be used for the discrete case |
I guess this was also the reason why initially in this PR it was called |
|
For the Dirac measure there is no uncertainty, so the entropy must be zero. For a package defining probability distributions, it is pretty much a given that we follow https://en.wikipedia.org/wiki/Dirac_measure in definition and naming. |
Well... it's pretty much arbitrary that you choose the Dirac on the integers and not on the real line (in the latter case, the only reasonable definition of the entropy is -Inf as e.g. is the limit of concentrating normals). The other choice should be also implemented then, because it is missing and would be useful. I would suggest a different naming then (Dirac on the real line was the meaning given by Dirac itself). :-) |
|
Maybe Kronecker is a better name for the discrete case. |
The discrete Dirac measure as defined and discussed in the comments above and on Wikipedia is not specific or limited to integers. The main point here is just which reference measure is used: if it is the counting measure (in case of the discrete definition) or the Lebesgue measure (in the limiting case of a Normal distribution). In contrast to Measures, in Distributions it is not possible to specify it explicitly. I think it is natural to use the counting measure as reference measure here. It is also more useful for practical applications, I assume, since otherwise all values of the logpdf are infinite. |
I don't follow
I agree that the discrete case is also useful, I would just avoid calling it Dirac because it would be incompatible with the original use. |
No, X can be anything. It is just a special case of And therefore I think the discrete Dirac measure should be a DiscreteDistribution whose pdf etc are defined with respect to the counting measure. Dirac measure is a common term for this distribution and its support is not limited to integers. What I wanted to point out above with the reference measures was mainly that the different entropy values are not surprising and arise from the different reference measures which induce the "fake" (i.e. non-existing) density of Inf at x and -Inf everywhere else when you talk about the Lebesgue measure and normal distributions (which is also what you end up with if you evaluate |
What you wrote is in contradiction with the documentation of Distributions (also ValueSupportThe
|
|
So a bit ex cathedra if you allow: Definition of a distribution Our definitions of |
|
We should leave |
|
Our definitions of DiscreteUnivariateDistribution and
CountinousUnivariateDistributions are sloppy, because not every measure
is discrete or continuous,
https://en.wikipedia.org/wiki/Lebesgue%27s_decomposition_theorem#Refinement
. But for a truly discrete measure we do use
DiscreteUnivariateDistribution.
The definition given in the documentation is not sloppy (you could argue
that you don't like the names, I don't think that is so important as long
as everything is well defined). You seem to choose not to adopt it. If this
is the official take, I think the documentation should be changed
accordingly.
I am well aware that some distributions cannot be represented in the
current system if you follow your interpretation... I even gave an example
which would be useful for me. I think that this is a much larger drawback
than what you gain by using your interpretation.
You seem to have thought more about this than me, but what is the advantage
of putting the fact that the support is countable into the type system?
… |
|
As said, things don't match, but in the end, |
Let me be more direct: there is no plan to support distributions that are neither? |
|
replaced by #1231 |
It is sometimes useful to have a degenerate distribution with a single value for generality, so that not to special-case constant values vs distributions. Let me know if you agree with this addition, then I will implement the remaining functions and add a docstring.