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

DiscreteNonParametric should be <: ContinuousDistribution #887

Open
abraunst opened this issue May 10, 2019 · 10 comments
Open

DiscreteNonParametric should be <: ContinuousDistribution #887

abraunst opened this issue May 10, 2019 · 10 comments

Comments

@abraunst
Copy link

abraunst commented May 10, 2019

The documentation at https://github.com/JuliaStats/Distributions.jl/blob/master/docs/src/types.md declares that distributions with domain in the reals should be subtypes of ContinuousDistribution. However, DiscreteNonParameteric in https://github.com/JuliaStats/Distributions.jl/blob/master/src/univariate/discrete/discretenonparametric.jl is declared as subtype of DiscreteDistribution, violating this rule. This seems a bug (see also my comments at the end of #332).

@abraunst abraunst changed the title DiscreteNonParametric should be <: Continuous DiscreteNonParametric should be <: ContinuousDistribution May 11, 2019
@matbesancon
Copy link
Member

Agreed on that.
Sort-of related to #771

@richardreeve
Copy link
Contributor

This also seems to be related to #916 though that PR actually only adds non-real support. It would be good to fix this ValueSupport bug, and I don't see why the Discrete subtype has to have Int elements at all, it just needs to have a finite (countably infinite?) number of whatever its element type is.

@mschauer
Copy link
Member

mschauer commented Jul 9, 2019

At this point where we need to define Discrete <: Continuous , we become aware that we all want to abolish the type params :-D

@richardreeve
Copy link
Contributor

richardreeve commented Jul 9, 2019

Well, I have to admit I'm not thinking of going quite that far! I was imagining DiscreteSupport <: DiscontinuousSupport <: ValueSupport and ContinuousSupport <: ValueSupport, and allowing DiscreteSupport to allow support over any countable set, whereas ContinuousSupport is continuous (and therefore uncountable), and DiscontinuousSupport is a statement about having Dirac deltas in the density function (or in the discrete support case, really just having a probability mass function instead).

@DilumAluthge
Copy link

Yeah, I would argue that "discrete" doesn't mean "the support is a subset of integers", but rather "the support is a countable set".

@mschauer
Copy link
Member

And continuous does not mean "support is a subset of the reals" but should mean perhaps that the distribution has a density (we are not going to model cantor distributions...)

@richardreeve
Copy link
Contributor

Does continuous require that support is a Number though? That would cover a lot of bases (including complex numbers, Unitful numbers, etc.), though we would also need the underlying number type (? I'm not sure there's anything that extracts this at the moment) to be <: AbstractFloat I imagine?

@mschauer
Copy link
Member

Just to state it clearly: The type system needs fixing, but DiscreteNonParametric should not be <: ContinuousDistribution

@richardreeve
Copy link
Contributor

The solution I proposed last year (to Dirac and all other distributions like this) in #941, #945 and ultimately #951 was to overhaul the whole system and fix all this, but there after several iterations (and months), there were always new minor issues being raised until I had no more time to work on it and so there was never enough enthusiasm to merge it. It’s a shame how much time was wasted.

@abraunst
Copy link
Author

abraunst commented Dec 2, 2020

The solution I proposed last year (to Dirac and all other distributions like this) in #941, #945 and ultimately #951 was to overhaul the whole system and fix all this, but there after several iterations (and months), there were always new minor issues being raised until I had no more time to work on it and so there was never enough enthusiasm to merge it. It’s a shame how much time was wasted.

For what is worth, I think that yours is the right way. 👍

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

No branches or pull requests

5 participants