Simplify pullback calls in the reverse mode #802

PetroZarytskyi · 2024-03-05T22:46:20Z

This PR simplifies both the code of RMV::VisitCallExpr and the code generated by it. In particular, it replaces the _grad/_r variable pairs with single _r variables. The PR also removes some dead code from RMV::VisitCallExpr, addresses clang-tidy warnings triggered by it, and improves its test coverage.
Fixes #801.

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

lib/Differentiator/VisitorBase.cpp

lib/Differentiator/ReverseModeVisitor.cpp

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

lib/Differentiator/VisitorBase.cpp

parth-07 · 2024-03-07T12:05:25Z

The work in this pull request seems good. Thank you for proactively trying to improve one of the most complicated components in the codebase. However, I think we may need to look at the issue and the solution more closely.

Please correct me if I am wrong anywhere. My autodiff skills are a bit rusty now.

Given a function:

double do_something(double p_u, double p_v) { ... }

Considering the primal code is r = do_something(u, v), then previously, we used to differentiate a call to do_something as follows:

double _r_d0 = _d_res;
double _grad0 = 0;
double _grad1 = 0;
do_something_pullback(u, v, _r_d0, _grad0, _grad1);
double _r0 = _grad0;
double _r1 = _grad1;
*_d_u += _r0;
*_d_v += _r1

With your patch, the same call will be differentiated as:

double _r_d0 = _d_res;
do_something_pullback(u, v, _r_d0, &*_d_u, &*_d_v);

The above two differentiated codes look similar and it appears that they should bring the same result mathematically. However, that's not the case. This is because of two reasons:

a function call hides assignment operations: assigning values of arguments to parameters
Argument and parameters are different mathematical variables and therefore they should not share adjoint variables.

Let's look at what happens in a function call more closely.

Primal code

r = do_something(u, v); // u and v are passed by value

This call can be made more clear by expanding it as follows:

double p_u = u;
double p_v = v;

do_something(p_u, p_v); // p_u, and p_v are passed by reference

In the original case, u and v are passed by value and assignments to p_u and p_v happens implicitly. We cannot see it but it's very much there. Now, why does this matter? In the second case, can we use the adjoint variables _d_u and _d_v for representing the adjoints of p_u and p_v? No, of course not. We need different adjoint variables for p_u and p_v because p_u is not the same variable as u and p_v is not the same variable as v. Using the same logic, we cannot use the adjoints of u and v when passing grad variables to do_something_pullback.

A practical example where this distinction matters:

#include "clad/Differentiator/Differentiator.h"
#include <iostream>
#define show(x) std::cout << #x << ": " << x <<"\n";

double reset(double u) {
    u = 0;
    return u;
}

double fn(double u, double v) {
    double res = u + v;
    res += reset(u);
    res += u;
    return res;
}

int main() {
    auto fn_grad = clad::gradient(fn);
    double u = 3, v = 5, du = 0, dv = 0;
    fn_grad.execute(u, v, &du, &dv);
    show(du);
    show(dv);
}

With this patch, the above code (incorrectly) outputs:

du: 1
dv: 1

The correct output is:

du: 2
dv: 1

Please let me know your thoughts on this.

parth-07

Please see the above comment for details.

PetroZarytskyi · 2024-03-08T14:20:46Z

@parth-07
Hi Parth. Thank you for your comment. I agree with your reasoning, I got confused. I still think this PR can be fixed to actually simplify things without breaking anything. Even though the changes will not be that drastic. But it needs more work.

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

parth-07 · 2024-03-12T20:34:15Z

I still think this PR can be fixed to actually simplify things without breaking anything.

That's great! It's important to simplify the call differentiation component. It has grown really complex over the years.

codecov · 2024-03-12T23:03:43Z

Codecov Report

Attention: Patch coverage is 97.67442% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.95%. Comparing base (8a77f81) to head (b340d05).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage   94.86%   94.95%   +0.08%     
==========================================
  Files          49       49              
  Lines        7357     7468     +111     
==========================================
+ Hits         6979     7091     +112     
+ Misses        378      377       -1

Files	Coverage Δ
include/clad/Differentiator/CladUtils.h	`100.00% <ø> (ø)`
include/clad/Differentiator/ErrorEstimator.h	`100.00% <ø> (ø)`
...e/clad/Differentiator/MultiplexExternalRMVSource.h	`100.00% <ø> (ø)`
include/clad/Differentiator/ReverseModeVisitor.h	`97.87% <ø> (ø)`
include/clad/Differentiator/VisitorBase.h	`100.00% <ø> (ø)`
lib/Differentiator/CladUtils.cpp	`93.00% <100.00%> (-3.75%)`	⬇️
lib/Differentiator/ErrorEstimator.cpp	`99.02% <100.00%> (ø)`
lib/Differentiator/MultiplexExternalRMVSource.cpp	`90.52% <100.00%> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`97.29% <100.00%> (+0.70%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`97.74% <ø> (-0.15%)`	⬇️
... and 1 more

... and 2 files with indirect coverage changes

Files	Coverage Δ
include/clad/Differentiator/CladUtils.h	`100.00% <ø> (ø)`
include/clad/Differentiator/ErrorEstimator.h	`100.00% <ø> (ø)`
...e/clad/Differentiator/MultiplexExternalRMVSource.h	`100.00% <ø> (ø)`
include/clad/Differentiator/ReverseModeVisitor.h	`97.87% <ø> (ø)`
include/clad/Differentiator/VisitorBase.h	`100.00% <ø> (ø)`
lib/Differentiator/CladUtils.cpp	`93.00% <100.00%> (-3.75%)`	⬇️
lib/Differentiator/ErrorEstimator.cpp	`99.02% <100.00%> (ø)`
lib/Differentiator/MultiplexExternalRMVSource.cpp	`90.52% <100.00%> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`97.29% <100.00%> (+0.70%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`97.74% <ø> (-0.15%)`	⬇️
... and 1 more

... and 2 files with indirect coverage changes

github-actions · 2024-03-13T13:05:26Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-03-13T21:44:03Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-03-13T22:08:39Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-03-13T23:52:08Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-03-14T11:25:34Z

clang-tidy review says "All clean, LGTM! 👍"

parth-07 · 2024-03-14T15:18:44Z

Hi @PetroZarytskyi

Can you please add more details in the pull-request description / commit-message regarding how the pullback calls are being simplified?

PetroZarytskyi · 2024-03-14T15:59:27Z

Hi @parth-07, I updated the PR message as well as the #801 issue.

github-actions · 2024-03-14T16:27:25Z

clang-tidy review says "All clean, LGTM! 👍"

…riables and not storing pointer args

…th iterators

github-actions · 2024-03-14T18:30:52Z

clang-tidy review says "All clean, LGTM! 👍"

parth-07

Looks good apart from the few minor comments.

include/clad/Differentiator/CladUtils.h

parth-07 · 2024-03-16T08:56:39Z

lib/Differentiator/CladUtils.cpp

+      // The argument is passed by reference if it's passed as an L-value.
+      // However, if arg is a MaterializeTemporaryExpr, then arg is a
+      // temporary variable passed as a const reference.
+      bool isRefType = arg->isLValue() && !isa<MaterializeTemporaryExpr>(arg);


I don't understand why is lvalue a reference type?

int a = b; // a is an l-value, but not a reference. int &a_ref = a; // a_ref is an l-value and a reference.

arg is supposed to be the argument expression passed to the function. If the function expects a ref-type argument, then arg is an l-value (usually a DeclRefExpr). But when it expects a non-ref type argument, it is implicitly converted to an r-value. The AST of arg will look somewhat like this:

ImplicitCastExpr <l-value to r-value> -DeclRefExpr

So arg will be an r-value. At least this is my understanding.

include/clad/Differentiator/VisitorBase.h

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

github-actions · 2024-03-18T08:21:39Z

clang-tidy review says "All clean, LGTM! 👍"

PetroZarytskyi force-pushed the simplify-pullback branch from 3c552ec to b573371 Compare March 5, 2024 22:54

github-actions bot reviewed Mar 5, 2024

View reviewed changes

vgvassilev reviewed Mar 6, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

vgvassilev requested a review from parth-07 March 6, 2024 06:02

vgvassilev reviewed Mar 6, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

PetroZarytskyi force-pushed the simplify-pullback branch from b573371 to 3aa40e5 Compare March 6, 2024 16:49

github-actions bot reviewed Mar 6, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

lib/Differentiator/VisitorBase.cpp Outdated Show resolved Hide resolved

parth-07 requested changes Mar 7, 2024

View reviewed changes

PetroZarytskyi force-pushed the simplify-pullback branch from cb3f1a4 to e779baf Compare March 12, 2024 15:06

github-actions bot reviewed Mar 12, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

PetroZarytskyi force-pushed the simplify-pullback branch 3 times, most recently from 836aa4f to b813c44 Compare March 12, 2024 22:56

PetroZarytskyi force-pushed the simplify-pullback branch 2 times, most recently from 20c252f to 19e5e9e Compare March 13, 2024 10:51

PetroZarytskyi force-pushed the simplify-pullback branch from 0044e43 to 067ab71 Compare March 13, 2024 21:30

PetroZarytskyi force-pushed the simplify-pullback branch from 70a186d to 51afd38 Compare March 13, 2024 23:31

vgvassilev requested a review from parth-07 March 14, 2024 06:52

PetroZarytskyi force-pushed the simplify-pullback branch from f8e50b7 to 80034ea Compare March 14, 2024 16:12

PetroZarytskyi added 9 commits March 14, 2024 20:11

Simplify pullback calls by replacing _grad/_r pairs with single _r va…

b0dd506

…riables and not storing pointer args

Expect a SourceLocation assertion in ArrayInputsReverseMode.C

76246b7

Don't expect a SourceLocation assertion in UserDefinedTypes.C

f510256

Remove pointer arithmetic from VisitCallExpr by replacing pointers wi…

bed0fbf

…th iterators

Address clang-tidy complaints

496e5e6

Improve test coverage of VisitCallExpr

9648f43

Fix recursive call differentiation and add a test

9aadd8f

Move the checks of call arguments for being temporary exprs to a routine

8c6d93c

Remove redundant checks from VisitCallExpr

453043e

PetroZarytskyi force-pushed the simplify-pullback branch from 80034ea to 453043e Compare March 14, 2024 18:15

parth-07 approved these changes Mar 16, 2024

View reviewed changes

vgvassilev assigned PetroZarytskyi Mar 16, 2024

vgvassilev added this to the v1.5 milestone Mar 16, 2024

github-actions bot reviewed Mar 18, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

Move GetMultiArgCentralDiffCall to ReverseModeVisitor

b340d05

PetroZarytskyi force-pushed the simplify-pullback branch from 49a1593 to b340d05 Compare March 18, 2024 08:04

vgvassilev merged commit 7cbefdf into vgvassilev:master Mar 18, 2024
88 checks passed

PetroZarytskyi deleted the simplify-pullback branch March 19, 2024 13:43

vaithak mentioned this pull request May 14, 2024

Regression in support of functions with unknown definition #890

Closed

vaithak mentioned this pull request Jun 4, 2024

Improve derived variable init for const pointers #919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify pullback calls in the reverse mode #802

Simplify pullback calls in the reverse mode #802

PetroZarytskyi commented Mar 5, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

parth-07 commented Mar 7, 2024 •

edited

Loading

parth-07 left a comment

PetroZarytskyi commented Mar 8, 2024

github-actions bot left a comment

parth-07 commented Mar 12, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 14, 2024

parth-07 commented Mar 14, 2024

PetroZarytskyi commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

parth-07 left a comment

parth-07 Mar 16, 2024

PetroZarytskyi Mar 16, 2024

github-actions bot left a comment

github-actions bot commented Mar 18, 2024

Simplify pullback calls in the reverse mode #802

Simplify pullback calls in the reverse mode #802

Conversation

PetroZarytskyi commented Mar 5, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

parth-07 commented Mar 7, 2024 • edited Loading

parth-07 left a comment

Choose a reason for hiding this comment

PetroZarytskyi commented Mar 8, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

parth-07 commented Mar 12, 2024

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

github-actions bot commented Mar 14, 2024

parth-07 commented Mar 14, 2024

PetroZarytskyi commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

github-actions bot commented Mar 14, 2024

parth-07 left a comment

Choose a reason for hiding this comment

parth-07 Mar 16, 2024

Choose a reason for hiding this comment

PetroZarytskyi Mar 16, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2024

PetroZarytskyi commented Mar 5, 2024 •

edited

Loading

parth-07 commented Mar 7, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading