search

stripe / rainier

Bayesian inference in Scala.
https://rainier.fit
Apache License 2.0
433 stars 51 forks source link

Customize zip implementation for RandomVariable #374

Closed sritchie closed 5 years ago

sritchie commented 5 years ago

No need to build a few closures if we can help it. This PR builds the zipped instance directly.

avi-stripe commented 5 years ago

Can you say more about the usage that is leading to RandomVariable construction being in the performance-sensitive path? In general my assumption has been that any code in RV is only run once, at model construction time, vs 1000s or millions of times at sampling time, and so would have to be really egregious to worry about optimizing (and this clearly isn't that).

sritchie commented 5 years ago

Fair, it's not - I just felt that this construction was more obvious and cleaner than the nested flatMap call, in addition to being more efficient (though as you point out, that doesn't matter). Feel free to reject.

But it does look nicer / clearer.

avi-stripe commented 5 years ago

I'm fine with it - I'll merge it now. Just worried that I was missing something about performance implications.