Allow arrays in pm.Bound

[aseyboldt]

pm.Bound doesn't support array valued bounds at the moment. This is fixed in this PR. I also refactored the Bounded.__init__ function a bit.

[junpenglao]

should it be tensor (float or 1D vector)?

[aseyboldt]

yes, arbitrary shape actually.

[junpenglao]

LGTM. The broken example garch_example.py is also fixed with this PR. #2254

[fonnesbeck]

We probably want an example of this. Maybe just in the examples folder, rather than a full-blown notebook.

[junpenglao]

We can edit the example censored_data.py

[junpenglao]

There seems to be some problem with the implementation, I tested on the Imputed censored model in censored_data.py:
1, Got ValueError: Bad initial energy: inf. The model might be misspecified. using ADVI init
2, Estimation is off (with init=None):
Before:

After:

[aseyboldt]

By "before/after" do you mean before and after applying this patch?

[ferrine]

Is it possible to make Bound to be a subclass of distribution?

[aseyboldt]

Calling a Bound object produces a Bounded object, which is a distribution subclass. (It's basically a distribution factory). So I'm not sure what you mean. Do you want to change the API for Bound? @ferrine

[junpenglao]

By "before/after" do you mean before and after applying this patch?

@aseyboldt Yep

[aseyboldt]

That was a smart one. I forgot to pass the transformation to the wrapped distribution....

[ferrine]

@aseyboldt issubclass(Bound(Normal, -1, 1), Distribution)

[aseyboldt]

@ferrine No, that shouldn't be true. If anything it should be
issubclass(Bound(Normal, -1, 1), Distribution), but that would mean that we create types on the fly and that often clashes with pickle. We have
isinstance(Bound(Normal, -1, 1).dist(mu=0, sd=1), Distribution) . (not tested, I have other stuff running on a different branch)

[ferrine]

You can, just use __reduce__ (https://docs.python.org/3/library/pickle.html#object.__reduce__)
It can pickle arbitrary things created on the fly.

[aseyboldt]

I didn't say it can't be done. But I think it is trouble and doesn't do anything useful.

[ferrine]

Sorry if I misunderstood you. It adds consistency in the structure. It is confusing to have failing subclass check for implementation stuff that works with distributions

[aseyboldt]

I think we shouldn't depend on any subclass checks. That is typically bad practice, too. If we need to find out more about the distribution/RV, we could add a property kind or similar, that could be 'discrete' or 'continuous'. Or maybe better domain, which could be reals, integers...

[twiecki]

@aseyboldt can you remove the conflicts? would like to merge this into the 3.2 branch.

[junpenglao]

There is failed test as well.

[aseyboldt]

I think this is ready to merge.
@ferrine I did end up making sure the inheritance with Discrete and Continuous checks out right, I think you were right about that.

[junpenglao]

@aseyboldt pm.Bound now also accept tensor as bound right? Could you also write a test for that?

[ferrine]

How do you feel about returning dynamicly created class that can be ofc pickled with __reduce__? I saw code above and that can be not so difficult and take little coding

[ferrine]

I feel worried about this extra Bound class that does nothing and is not Distribution itself but proposed to be the one

[aseyboldt]

I'd rather not make this any more complicated than it already is.
Sadly, we don't distinguish properly between random variables and distributions, but use the constructor of our distribution objects to build random variables (observed and otherwise). This leads to the somewhat strange behaviour

issubclass(pm.Normal, pm.Distribution)  # -> True
isinstance(pm.Normal('a'), pm.Distribution) # -> False
isinstance(pm.Normal('a'), pm.Normal) # -> False

As we really don't want to second to be true, I think the first actually shouldn't be true in the first place. We shouldn't think of our distribution classes as distributions, but as factory functions for random variables. The distributions are pm.Normal.dist.
So I kind of don't see the point of adding another non-standard thing on top of the metaclasses and overriden __new__ that's already in there, to make a subclass check true, that shouldn't be true in the first place. Writing it isn't the problem, it just makes the code ever harder to read.

We do have the following now, which I think is the important part:

BoundNormal = pm.Bound(pm.Normal)
isinstance(BoundNormal.dist(), pm.Continuous) # -> True

The original design of pm.Bound is I guess a bit unfortunate, we wouldn't have that problem in the first place if it took a distribution as parameter and returned a random variable:

dist = pm.Normal.dist(mu=5, sd=3)
pm.Bound("a", dist, lower=3)

[ferrine]

Sounds reasonable. Hope no one will meet the problem of that kind

[junpenglao]

@aseyboldt for the thing you mention above:

dist = pm.Normal.dist(mu=5, sd=3)
pm.Bound("a", dist, lower=3)

Could we not use tt.clip to do the same thing?

[aseyboldt]

No, clip would be more like censoring, as the values below lower are just set to lower. Bound mostly sets an appropriate transformation.

[aseyboldt]

@junpenglao I just pushed a test for tensor bounds.

[junpenglao]

Thanks @aseyboldt