kaskr / adcomp

AD computation with Template Model Builder (TMB)
Other
176 stars 80 forks source link

lgamma() / lfactorial() not taking expressions #325

Open lentinj opened 4 years ago

lentinj commented 4 years ago

Description:

Providing an expression argument lgamma(), amongst others, doesn't compile.

Reproducible Steps:

Try compiling the following example:

#include <TMB.hpp>

template<class Type>
Type objective_function<Type>::operator() () {
    DATA_VECTOR(x);

    // Works
    vector<Type> out1 = lgamma(x);
    REPORT(out1)

    // Doesn't compile!
    vector<Type> out2 = lgamma(x + 0);
    REPORT(out2)

    // Works
    auto lgamma_vec = [](vector<Type> x) -> vector<Type> {
        return lgamma(x);
    };
    vector<Type> out3 = lgamma_vec(x + 0);
    REPORT(out3)

    return 0;
}

The above code will only compile if the second example is commented out.

I (think!) this is due to function templates interacting badly with the lazy evaluation of x + 0. As it is handed one template expects to also return a "something + something" expression, which it can't do.

The bodge in the third case uses the same Type as the objective_function, so the lazy evaluation is no longer a problem.

Is there a better approach here I'm missing?

TMB Version:

> packageVersion("TMB")
[1] ‘1.7.18’

R Version:

> R.version.string
[1] "R version 3.6.3 (2020-02-29)"

Operating System:

$ uname -v
#1 SMP Debian 5.7.6-1 (2020-06-24)
kaskr commented 4 years ago

It's a known issue. An alternative 'fix' is:

vector<Type> out2 = lgamma(vector<Type>(x + 0));

I admit it would be nice if such casts weren't necessary.

lentinj commented 4 years ago

Oops, yes that would have been the sensible thing to do. Apologies.

Happy to make a pull request, adding a note to the docs to save future head-scratching. The most useful place I can see is http://kaskr.github.io/adcomp/_book/Errors.html#compilation-errors - does this seem like a plan? I can't any specific docs on the vectorized lgamma, and it's not strictly an lgamma problem anyway.

kaskr commented 4 years ago

@lentinj

does this seem like a plan?

Yes - thanks.