vgvassilev / clad

clad -- automatic differentiation for C/C++
GNU Lesser General Public License v3.0
289 stars 122 forks source link

Use the `std::vector<>` namespace when declaring a size_type var for STL containers #1072

Open kchristin22 opened 2 months ago

kchristin22 commented 2 months ago
#include "clad/Differentiator/Differentiator.h"
#include "clad/Differentiator/STLBuiltins.h"
// #include "../TestUtils.h"
// #include "../PrintOverloads.h"

#include <vector>

double fn(double u) {
    std::vector<double> vec;
    vec.push_back(u);
    return vec[0];
}

int main() {
    auto grad = clad::gradient(fn);
}

Clad generates the derivative like so:

void fn_grad(double u, double *_d_u) {
    std::vector<double> _d_vec({});
    std::vector<double> vec;
    double _t0 = u;
    std::vector<double> _t1 = vec;
    clad::custom_derivatives::class_functions::push_back_reverse_forw(&vec, u, &_d_vec, *_d_u);
    std::vector<double> _t2 = vec;
    clad::ValueAndAdjoint<double &, double &> _t3 = clad::custom_derivatives::class_functions::operator_subscript_reverse_forw(&vec, 0, &_d_vec, _r0);
    {
        size_type _r0 = 0UL; // default namespace is std and size_type is not included in there
        clad::custom_derivatives::class_functions::operator_subscript_pullback(&_t2, 0, 1, &_d_vec, &_r0);
    }
    {
        u = _t0;
        clad::custom_derivatives::class_functions::push_back_pullback(&_t1, _t0, &_d_vec, &*_d_u);
    }
}

As the comment suggests, size_type is not a member of std, but is included inside std::vector. So this line should be altered to:

std::vector<double>::size_type _r0 = 0UL;
gojakuch commented 2 months ago

I think this is kind of a duplicate of #1050, right? if so, I'm working on this

kchristin22 commented 2 months ago

Yes, hadn't seen the issue. Thanks for noticing. Closing this.

gojakuch commented 2 months ago

actually, now that I think about this, it's not really a duplicate of #1050, since there I was talking about nested name qualifiers, this issue is about nested type qualifiers, if that's the right way to say it, so I'm reopening this. I apologise for the initial confusion. but I think it's a more general problem than just std::vector<>s, it's that we need to build the type qualifiers correctly in general.

also, for anyone who's gonna be solving this, this issue is version- and machine-dependent. on my machine and with my version of Clang, the generated type for the _r0 var in the original example is std::vector::size_type which is also incorrect because it lacks the template specification for the vector class, however it's different from the result demonstrated above.

vgvassilev commented 2 months ago

I suspect we can get away most of the time using auto.

gojakuch commented 2 months ago

true, I've thought about this as well. but is it possible to generate code with auto from the AST? I thought auto types are removed from the code pretty early into the compilation. even stuff like auto lambda = [](){} is printed from the visitor with a synthetic type name placeholder instead of auto so that was my reason to believe we cannot really rely on auto.

gojakuch commented 2 months ago

I think utils::AddNamespaceSpecifier could be ustilised to solve this issue if auto is not an option. but if there's a way to use auto, we probably should use it more, although I'm not sure if it's safe to always do that actually

vgvassilev commented 2 months ago

true, I've thought about this as well. but is it possible to generate code with auto from the AST? I thought auto types are removed from the code pretty early into the compilation. even stuff like auto lambda = [](){} is printed from the visitor with a synthetic type name placeholder instead of auto so that was my reason to believe we cannot really rely on auto.

It is possible to generate such types assuming that we know the right hand side of the assignment. In some cases auto is preferable, such as Kokkos, where the intent is to hide the implementation details and allow things to recompile for other platforms/architectures. On the other hand, using excessively auto makes the code quite unreadable and we will need to be careful to not overuse it.