Remove grad and gradx members from variable exprs

[jan-grimo]

Implementation of an idea from #160 removing grad and gradx members from VariableExpr in favor of value and expression pointers to which updates can be written during expression tree traversal.

Fixes the associated regressions in derivative calculation, too. The tests no longer leak memory.

Some details:

• In principle, there's no need to add bind_value and bind_expr to the Expr<T> interface. For instance expr.bind_value(nullptr) can be replaced with a static_cast<VariableExpr<T>*>(expr.get())->gradPtr = nullptr; because this is only ever called on variable expressions. Any preference you might have there is easy to adopt.
• The nullptr checking in VariableExpr<T>::propagate and ::propagatex could just be replaced with an assert for debug build checking if you have runtime performance concerns.

[allanleal]

Hi Jan, thanks a lot for the fix. In the end you managed to find a solution that required very little changes. Very good. I have just kindly requested you to add a short comment to two methods below, just to make it clear what are their roles. I've added a memory leak check in the CI.

[allanleal]

Please add a short (Doxygen ///) comment here for these bind_* functions. I like your approach.

[jan-grimo]

I'm glad you approve!

By the way, you could cut valgrind out of the CI and use sanitizers instead, with something like the following in tests/CMakeLists.txt:

if(AUTODIFF_SANITIZE_TESTS)
  target_compile_options(autodiff-cpptests PRIVATE "-fsanitize=address,undefined")
  target_link_options(autodiff-cpptests PRIVATE "-fsanitize=address,undefined")
endif()

... Although that only works for linux and mac and would need safing against windows

[allanleal]

Interesting. I'm OK with either approach. If the CI fails again, could you please introduce this then (the AUTODIFF_SANITIZE_TESTS logic)?

…

[allanleal]

Setting AUTODIFF_SANITIZE_TESTS to ON in the macOS build is failing. I'll probably enable this only for Linux, which should be enough.

[jan-grimo]

Yeah, I think the cmake on osx doesn't much like your use of a target named tests. (That's not it). If you don't mind, I'm gonna try to fix that.

MSVC did add Asan a while ago, so I thought it might be worth trying via the compiler flag testing. Not sure what's been failing the CI pipeline though.

[allanleal]

I don't think this works in Windows. I'll remove this (the only thing missing to get CI fully passing)

[adam-ce]

hi,
i'm using autodiff reverse mode to unit test my own manual gradient derivatives for complex functions.

I do functions similar to the following
template <typename scalar_t, int DIMS> EXECUTION_DEVICES glm::mat<DIMS, DIMS, scalar_t> inverse(const glm::mat<DIMS, DIMS, scalar_t>& cov, const glm::mat<DIMS, DIMS, scalar_t>& incoming_grad);
which i want to unit test.
The thing is, that i have several in- and outputs, and i'm manually var.expr->propagate(grad) on each of the components and then reading the result from the input vars one by one in a loop. Something else that i do, is casting the expressions to variables, and then reading intermediate results of the gradient while debugging. that's now not possible any more. i think that the easiest workaround for me is to create my own autograd variable and call bind myself.

I understand the reasons for the changes, but if i understand it correctly, it could have been limited to the gradx case. imo it's also a nicer design, if you can get the gradient from the variable directly. anyway, what i actually want to say, is, that it would be nice if you also consider such use cases, i.e., don't try to completely hide the implementation, because that makes it harder to adapt for certain use cases (another example is, that some important functionality is now in detail, which also broke my code).

[allanleal]

Hi Adam, many thanks for reporting this. You're right that the memory leak issue was caused only when gradx and propagatex were used (i.e., for higher-order derivatives).

I would accept changes that revert the grad and propagate methods. I won't be available for this soon. Would you kindly contribute with a PR that introduces back grad and propagate?