-
Notifications
You must be signed in to change notification settings - Fork 421
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
Comments
Agreed on that. |
This also seems to be related to #916 though that PR actually only adds non-real support. It would be good to fix this |
At this point where we need to define |
Well, I have to admit I'm not thinking of going quite that far! I was imagining |
Yeah, I would argue that "discrete" doesn't mean "the support is a subset of integers", but rather "the support is a countable set". |
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...) |
Does continuous require that support is a |
Just to state it clearly: The type system needs fixing, but |
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. 👍 |
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 ofDiscreteDistribution
, violating this rule. This seems a bug (see also my comments at the end of #332).The text was updated successfully, but these errors were encountered: