Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kchristin22 opened this issue Sep 3, 2024 · 7 comments

Comments

@kchristin22
Copy link
Collaborator

#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;
@kchristin22 kchristin22 changed the title Use the std::vector namespace when declaring a size_type var for STL containers Use the std::vector<> namespace when declaring a size_type var for STL containers Sep 3, 2024
@gojakuch
Copy link
Collaborator

gojakuch commented Sep 4, 2024

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

@kchristin22
Copy link
Collaborator Author

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

@gojakuch
Copy link
Collaborator

gojakuch commented Sep 9, 2024

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.

@gojakuch gojakuch reopened this Sep 9, 2024
@vgvassilev
Copy link
Owner

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

@gojakuch
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner

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.

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

No branches or pull requests

3 participants