Skip to content

Deprecated Constant class#2452

Merged
springcoil merged 10 commits intomasterfrom
continuous_constant
Aug 11, 2017
Merged

Deprecated Constant class#2452
springcoil merged 10 commits intomasterfrom
continuous_constant

Conversation

@fonnesbeck
Copy link
Member

@fonnesbeck fonnesbeck commented Jul 28, 2017

Fixes #2451

Also adds a bunch of newer distributions that are missing from the API docs.

@aseyboldt
Copy link
Member

Hm. Having the dirac measure as a subclass of Continuous seems strange from a mathematical point of view. It doesn't have a density.
I can't even think of a use case for this distribution, couldn't we always use a constant (optionally wrapped in a deterministic) instead? Unless someone knows one, I'd rather deprecate it.

@fonnesbeck
Copy link
Member Author

We've had this discussion in the past, but we can bring it up again. However, if someone wants to set a value of a constant to a floating point value they should be able to, and so it makes sense to move it here based on our taxonomy. Alternatively, we could have it not subclass from either Continuous or Discrete.

Moving it to Continuous is the easiest.

@aseyboldt
Copy link
Member

Wouldn't that mean that during auto assignment of step methods NUTS will try to sample from that?

@fonnesbeck
Copy link
Member Author

Correct. I was on your side the last time the issue was brought up, but folks argued strongly in favor of keeping it. It apparently does get used, as evidenced by the issue.

@aseyboldt
Copy link
Member

Well, then maybe it is best if it inherits from Distribution directly. That way it will at least not mess up step method assignment (I hope?). Although I wouldn't be surprised if we had some code that actually assumes that all distributions are either discrete or continuous.

@fonnesbeck
Copy link
Member Author

We won't know until we try ...

@fonnesbeck fonnesbeck changed the title Change Constant to be Continuous subclass Change Constant to be Distribution subclass Jul 28, 2017
@junpenglao
Copy link
Member

couldn't we always use a constant (optionally wrapped in a deterministic) instead?

I agree. Is it possible to just internally take the constant and wrapped in a deterministic?

@twiecki
Copy link
Member

twiecki commented Jul 29, 2017

Yeah, I don't recall the reasoning last time, but just because people are using it doesn't mean that there isn't a better way which we should support instead (i.e. a Deterministic with a float). I'm in favor of dropping it.

@fonnesbeck
Copy link
Member Author

Ok, I will leave it where it is (Deterministic) and add a deprecation warning.

@fonnesbeck fonnesbeck changed the title Change Constant to be Distribution subclass Deprecated Constant class Aug 4, 2017
@fonnesbeck
Copy link
Member Author

OK, after all that, we are just deprecating now.

@springcoil
Copy link
Contributor

Is this ready to merge?

@fonnesbeck
Copy link
Member Author

Please do, unless anyone objects

@springcoil springcoil merged commit 278c107 into master Aug 11, 2017
@springcoil
Copy link
Contributor

Done.

@michaelosthege
Copy link
Member

michaelosthege commented Sep 12, 2017

Hmmm. I'm the one who opened #2451 in the first place and now the deprecation of Constant is causing some real trouble for me.
Let me explain:
I'm building a package where a model is independent of the data. For sampling it, one takes a dataset, maps each instance (experiment) in the dataset to prior Distributions and then the package instantiates prior RVs and build the model (computation graph) for the dataset at hand.
To do this, I specify priors as pymc3.Distribution objects (eg. pymc3.Normal.dist("ned", mu=1, sd=2)) outside of any model context.

From that perspective, a Constant is a perfectly fine distribution!
But pymc3.Deterministic is a function, not a distribution. It can not exist outside of a model context. Which means one can no longer describe a constant distribution independent of a model context.

I was able to use pymc3.Deterministic by introducing some if/else in my code, but having the CompundStep skip constant RVs seems a much more elegant solution.

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.

6 participants