Implement ordinal regression GLM (ordered_logistic_glm_lpmf) #1252
Conversation
…stable/2017-11-14)
|
I measured speedups of this compared to using
|
| * @param y integer vector parameter | ||
| * @param x design matrix | ||
| * @param beta weight vector | ||
| * @param cuts vector of cutpoints |
There was a problem hiding this comment.
I assume this only accepts a column vector, since that's what we take "vector" to mean. Otherwise, please indicate this can be a row vector or column vector. We want the doc to be clear on types here since the templating isn't providing that information when you template the whole container.
I'm just adding single comments rather than reviewing as I don't want to overreview someone else who might be doing this.
| check_finite(function, "Final cut-point", cuts[N_classes - 2]); | ||
| check_finite(function, "First cut-point", cuts[0]); | ||
|
|
||
| if (size_zero(y, x, beta, cuts)) |
There was a problem hiding this comment.
This isn't your fault, but it's another function that landed without sufficient review---it should be named size_zero_any or something like that. Anyway, guess that's a different PR.
There was a problem hiding this comment.
Yeah, this one is on me. This got in before the _any discussion in the is_nan PR. Sorry about that. I am assigning myself to fix this one.
|
Whether multiplication is expensive or not depends on the context. It's slower than addition, but faster than exponentiation and much faster than a cache miss on memory.
Doesn't evaluating .(cut > 0) require branching. I can imagine it's better at allow vectorization than a literal branch in the code.
If you know how to do it, it would be great to speed up Stan's vectorized functions. As is, they're vectorized naively by just calling the function repeatedly. Not in this PR, of course!
|
|
cut>0 is just a comparison, not a branch. I am not sure, but I think comparisons can be vectorized. Explicitely vectorizing code requires use of intrinsics, which is tedious and not portable. Luckily Eigen does it, so we do not have to. I just assume all Eigen expression will produce vectorized binary code. While there might be exceptions, Eigen expessions seem to always results in equivalent or faster calculations than doing things element-wise. |
|
On May 28, 2019, at 2:54 PM, t4c1 ***@***.***> wrote:
cut>0 is just a comparison, not a branch. I am not sure, but I think comparisons can be vectorized.
That would be cool if they could be vectorized. It's not a branch in the if/then structure, but internally, it has to be doing a comparison and taking different behavior based on the result as that's the semantics. But it may be encapsulated such that it doesn't look like a branch to the compiler. That's why I was asking. My compiler knowledge isn't that deep.
Explicitely vectorizing code requires use of intrinsics, which is tedious and not portable. Luckily Eigen does it, so we do not have to. I just assume all Eigen expression will produce vectorized binary code. While there might be exceptions, Eigen expessions seem to always results in equivalent or faster calculations than doing things element-wise.
That seems to imply we should redo our vectorization to lean on whatever Eigen's using to do it. I'm not saying we should write our own proper vectorization library, just that we might be able to lean on Eigen's. You seem to be writing faster versions of our vectorized functions using Eigen. I'd like to expose that to users as elementwise application of functions happens everywhere.
|
That is not true. Comparisons are basic CPU instructions, just like for example adition. Just the result is either 0 or 1. Comparisons without if/loop are not branches. |
|
I think I see what you're getting at. Just because comparison has an intrinsically branching behavior, it doesn't entail generating branching instructions. Is the relevant feature here whether you need to jump for one of the branches?
On a side note, do you know if the ternary conditional operator (a ? b : c) generates branches?
There's a lot of bad folk wisdom around all this stuff and I've never dug into it deeply enough or found it made enough of a difference to show up in end-to-end profiling. So thanks for bearing with.
… On May 28, 2019, at 5:03 PM, t4c1 ***@***.***> wrote:
taking different behavior
That is not true. Comparisons are basic CPU instructions, just like for example adition. Just the result is either 0 or 1. Comparisons without if/loop are not branches.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Sorry, I don't understant, what do you mean with the first question. Ternary operator is a branch. On a side note I assume we want this GLM to have similar input types as softmax regression. |
|
Let me elaborate with an example. Let's say I have a naive implementation of absolute value using the ternary operator. The question is whether this generates branching instructions or not. How would that compare to the which shouldn't generate any branching. I saw a Stack Overflow discussion about this recently which said the first version's faster on modern compilers, but now I can't find it. |
|
FIrst one would generate a branch and second would not. But the second would compile into quite a number of instructions. However, both might be optimized by a compiler. |
|
This test failure was expected and will disappear once PR1249 gets merged. |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01) |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0) |
|
Do we have any volunteers to review this? Bob's and my intial comments were addressed so this is definitely ready for a proper review. |
There was a problem hiding this comment.
This looks great! I always learn a lot about using Eigen from your pulls.
The rev testing is very thorough, but I'm not sure what the policy on tests are for the prob functions (i.e. whether you also need prim/fwd/mix tests here) @syclik what's your view here?
|
What is going on with these tests, running for 22 hours? |
|
Its somethin with the AWS instances I think. Restarting the entire tests is the only way I know how to fix this. |
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98) |
|
@andrjohns This is ready for next review. |
Summary
This implements ordinal regression GLM (ordered_logistic_glm_lpmf) for CPU.
Tests
New tests are in
rev/mat/prob/ordered_logistic_glm_lpmf_test.cpp.Side Effects
None.
Checklist
Math issue Implement ordinal regression GLM (ordered_logistic_glm_lpmf) #1251
Copyright holder: Tadej Ciglarič, University of Ljubljana
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested