Skip to content

Comments

Optimized logpt computation#2959

Merged
ColCarroll merged 3 commits intopymc-devs:masterfrom
junpenglao:optimized_logpt
May 7, 2018
Merged

Optimized logpt computation#2959
ColCarroll merged 3 commits intopymc-devs:masterfrom
junpenglao:optimized_logpt

Conversation

@junpenglao
Copy link
Member

I change the logpt computation in #2949 to fix #2948, however, it slows down the speed as some graph optimization is turned off (those optimization is originally cause the error in #2948). I am trying with a differen approach here.
@ColCarroll

I change the logpt computation in #2949 to fix  #2948, however, it slows down the speed as some graph optimization is turned off (those optimization is originally cause the error in #2948). I am trying with a differen approach here.
@ColCarroll
@ColCarroll
Copy link
Member

I don't quite understand the problem: it looks like you are trying a more explicit loop to give the compiler a better chance of optimizing? I guess I also don't understand what caused the edge case in the first place, and whether it is ok to let that fail (that is the failing test)

@junpenglao
Copy link
Member Author

junpenglao commented Apr 29, 2018

Thanks for having a look @ColCarroll.
So the error is a bit difficult to trace down as it happens (i suspect) in the graph optimization. In that particular edge case (edge because it only doest work with sd being a vector in logNormal distribution, other distribution works fine), if you set the ValueGradientFunction to compile in python (mode='FAST_COMPILE') it works fine. My initial fix is to set the logpt computation to the older way, but seems in effect it skip the optimization in the step of the logpt computation step (that's why no more error in the edge case). So now what I am trying to do is be more explicit at these similar edge case and compile a function that turns off the C optimization.
What do you think?

@twiecki
Copy link
Member

twiecki commented Apr 30, 2018

Seems like a theano bug, no?

@junpenglao
Copy link
Member Author

@twiecki not sure, I cannot really pin down where exactly is the error yet... compiling a theano function using pm.Lognormal.dist().logp()+... etc works without problem

@ColCarroll ColCarroll merged commit 1ca35d6 into pymc-devs:master May 7, 2018
@ColCarroll
Copy link
Member

Ran the asv benchmarks locally, and this gets the hierarchical model running quicker again (160 ES/S vs 138 ES/S). Agree that it would be nice to understand more about why this happened, but I wanted 14% of my time back!

@junpenglao junpenglao deleted the optimized_logpt branch May 7, 2018 13:26
@junpenglao
Copy link
Member Author

So, this PR makes me quite paranoid so I did a bit more digging.
Turns out doing logp = tt.add(*map(tt.sum, factors)) is actually slightly faster in terms of raw speed, as the graph is smaller, but surprisingly in practice (I tested a few other models that I am working on) doing logp = tt.sum([tt.sum(factor) for factor in factors]) seems to be numerically more stable (hence the higher effective sample size / sampling time ratio in the benchmark).

All the output is in the folder below if you would like to test on your own model https://github.com/junpenglao/Planet_Sakaar_Data_Science/tree/master/Miscellaneous/theano_graph_optim

agustinaarroyuelo pushed a commit to agustinaarroyuelo/pymc3 that referenced this pull request Feb 8, 2019
* Optimized logpt computation

I change the logpt computation in pymc-devs#2949 to fix  pymc-devs#2948, however, it slows down the speed as some graph optimization is turned off (those optimization is originally cause the error in pymc-devs#2948). I am trying with a differen approach here.
@ColCarroll

* fix test
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.

Theano does not sample from HalfCauchy and Lognormal together

3 participants