Speed up to DIC and BPIC

[denadai2]

Fixes #2487

[springcoil]

LGTM.

Can someone explain to me why this is a significant speed up?

[ColCarroll]

This is maybe a bad design choice, but

https://github.com/pymc-devs/pymc3/blob/master/pymc3/model.py#L156

makes the logp look like an attribute, when it actually puts together a function based on the current state of the model. In this case the underlying state is not changing, so the function can be calculated just once, then reused.

I say maybe a bad design choice because it makes the code way more readable, and is only a problem when logp is computed in an inner loop without updating the model, as it is here.

[springcoil]

What would be the solution then? If this is a bad design choice?

[junpenglao]

@springcoil we also have more discussions here: #2480

[ColCarroll]

There's sort of three metrics at odds: performance for changing input, performance for static input, and ease of use. There are three approaches I can think of that address these to various amounts --
The "safe" option is to make this an honest function call -- not as easy to use, but performant in any situation. Something like:

logp = model.get_logp()
logp(pt)

The current and previous APIs are super easy to use, but only performant in one of the two situations: the old API used a call to memoize which does great in this situation, but terrible if the state keeps changing.

The current design is super maintainable, does terrible if the state stays constant, and great if the state keeps changing.

I haven't looked too closely, but I think it would be super brittle to optimize all three of those concerns, since you would probably have to track when state is changing.

[springcoil]

I'd agree that it's really hard to optimize all three of those concerns. It's kinda a multi-objective optimization problem - so difficult :)

As a prejudice - I'd aim for ease of maintainability and for handling situations were state is changing (because that's often the main use case for users in my understanding), and just accept that this is a negative part of the design for when state needs to be constant.

Maybe this needs to be written up somewhere in the documentation - or is this sufficient?

@twiecki @fonnesbeck @ferrine ?

[junpenglao]

We should put it in the API quick start http://docs.pymc.io/en/latest/notebooks/api_quickstart.html where model.logp is called.

[junpenglao]

Thanks for the fixed @denadai2 ;-)

[denadai2]

@junpenglao np!