Differentiate for loop condition expression #818

rohanjulka19 · 2024-03-12T01:02:23Z

Fixes #746 - Incorrect differentiation of loop conditions in reverse mode

github-actions · 2024-03-12T05:36:34Z

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

codecov · 2024-03-12T05:37:41Z

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.98%. Comparing base (ef668c7) to head (3bd70e3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
+ Coverage   93.95%   93.98%   +0.03%     
==========================================
  Files          55       55              
  Lines        7967     8009      +42     
==========================================
+ Hits         7485     7527      +42     
  Misses        482      482

Files	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	`97.87% <ø> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`97.30% <98.07%> (+<0.01%)`	⬆️

... and 1 file with indirect coverage changes

Files	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	`97.87% <ø> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`97.30% <98.07%> (+<0.01%)`	⬆️

... and 1 file with indirect coverage changes

vgvassilev · 2024-03-12T05:37:57Z

Can you add tests?

rohanjulka19 · 2024-03-12T08:36:00Z

yeah have added the same test case mentioned in the issue. Will that be alright ?

github-actions · 2024-03-12T11:47:20Z

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

github-actions · 2024-03-12T17:30:36Z

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

parth-07

The solution needs to be corrected. The for-loop condition should be differentiated in each iteration.
The solution in this pull-request generates incorrect derivative for the example show below:

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

double fn(double i, double j) {
    double res = 0;
    for (int c = 0; (res += i * j); ++c) {
        if (c == 2)
            break;
    }
    return res;
}

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

Output
du: 5
dv: 3

Expected Output
du: 10
dv: 6

rohanjulka19 · 2024-03-13T08:40:46Z

Oh right ok, will fix this

rohanjulka19 · 2024-03-13T10:24:27Z

hi @parth-07 for c == 2 condition du and dv will be 15 and 9 right ? cause the loop will run 3 times.

parth-07 · 2024-03-13T12:17:23Z

hi @parth-07 for c == 2 condition du and dv will be 15 and 9 right ? cause the loop will run 3 times.

Oh, yes. You are right.

rohanjulka19 · 2024-03-14T12:07:48Z

hi @parth-07, I have added the differentiation of condition inside the loop body and also at the end of the loop. This is done because for the value where condition is false, the condition is exeucted but the loop body is not executed. So we need to add a extra differentiation step after loop body. This does not seem to be working for when we break at 1st or 2nd iteration and I am not able to understand why exactly. Can you just help me out here ? Also is this the right approach here ?

This is the function being differentiated

double fn(double i, double j) {
  double res = 0;
  for (int c = 0; (res = i * j), c<5; ++c) {
       if(c == 0) 
          break;
   }
  return res;
}

void fn_grad(double i, double j, clad::array_ref<double> _d_i, clad::array_ref<double> _d_j) {
    double _d_res = 0;
    unsigned long _t0;
    int _d_c = 0;
    int c = 0;
    clad::tape<double> _t1 = {};
    clad::tape<bool> _t3 = {};
    clad::tape<unsigned long> _t4 = {};
    double res = 0;
    _t0 = 0;
    for (c = 0; clad::push(_t1, res) , ((res = i * j) , c < 5); ++c) {
        _t0++;
        bool _t2 = c == 0;
        {
            if (_t2) {
                {
                    clad::push(_t4, 1UL);
                    break;
                }
            }
            clad::push(_t3, _t2);
        }
        clad::push(_t4, 2UL);
    }
    goto _label0;
  _label0:
    _d_res += 1;
    {
        for (; _t0; _t0--)
            switch (clad::pop(_t4)) {
              case 2UL:
                ;
                {
                    _d_res += 0;
                    res = clad::pop(_t1);
                    double _r_d0 = _d_res;
                    _d_res -= _r_d0;
                    * _d_i += _r_d0 * j;
                    * _d_j += i * _r_d0;
                }
                --c;
                if (clad::pop(_t3)) {
                  case 1UL:
                    ;
                }
            }
        {
            _d_res += 0;
            res = clad::pop(_t1);
            double _r_d0 = _d_res;
            _d_res -= _r_d0;
            * _d_i += _r_d0 * j;
            * _d_j += i * _r_d0;
        }
    }
}

Output:
du: 0
dv:0

Expected:
du: 5
dv:3

github-actions

clang-tidy made some suggestions

github-actions · 2024-03-15T12:48:51Z

lib/Differentiator/ReverseModeVisitor.cpp

    if (FS->getCond())
-      cond = Visit(FS->getCond());
+      std::tie(condDiff, condExprDiff) = DifferentiateSingleExpr(FS->getCond());
+      std::tie(condDiffOuter, condExprDiffOuter) = DifferentiateSingleExpr(FS->getCond());


warning: misleading indentation; statement is not part of the previous 'if' [clang-diagnostic-misleading-indentation]

std::tie(condDiffOuter, condExprDiffOuter) = DifferentiateSingleExpr(FS->getCond()); ^

Additional context

lib/Differentiator/ReverseModeVisitor.cpp:1069: previous statement is here

if (FS->getCond()) ^

github-actions · 2024-03-15T12:48:51Z

lib/Differentiator/ReverseModeVisitor.cpp

    if (FS->getCond())
-      cond = Visit(FS->getCond());
+      std::tie(condDiff, condExprDiff) = DifferentiateSingleExpr(FS->getCond());
+      std::tie(condDiffOuter, condExprDiffOuter) = DifferentiateSingleExpr(FS->getCond());


warning: misleading indentation: statement is indented too deeply [readability-misleading-indentation]

std::tie(condDiffOuter, condExprDiffOuter) = DifferentiateSingleExpr(FS->getCond()); ^

Additional context

lib/Differentiator/ReverseModeVisitor.cpp:1069: did you mean this line to be inside this 'if'

if (FS->getCond()) ^

github-actions · 2024-03-15T13:09:04Z

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

rohanjulka19 · 2024-03-15T13:34:18Z

I differentiated cond twice adding one inside the loop and one outside the loop
Now RHS of comma operator is differentiated and added to block before LHS this is needed because if there are multiple condition expression the last one should decide the value to be differentiated.

github-actions · 2024-03-15T13:54:57Z

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

parth-07

I differentiated cond twice adding one inside the loop and one outside the loop

This is correct.

The below example gives wrong result:

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

double fn(double i, double j) {
    double res = 0;
    for (int c = 0; true && (res += i * j); ++c) {
        if (c == 0)
            break;
    }
    return res;
}

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

vgvassilev · 2024-03-20T15:48:18Z

@rohanjulka19, can you take a look at the comment and possibly make progress here?

rohanjulka19 · 2024-03-20T16:22:53Z

Hi @vgvassilev , yeah sorry should have acknowledged here, I had seen the comment. I am currently working on adding && operator support in the VisitBinaryOperator function it's just that I am facing a issue that I haven't figured out how to solve. Basically for expression a && b && c it needs to have concentric if conditions i.e if(a) { df/da if(b) {df/db} } but since we start parsing from rightmost expression I am unable to add the right hand side expression's differentiation inside the IF block of the left expression as I am not sure how to pass it to the left hand side expression when visiting the LHS expression. So was just figuring that out plus I had university work so wasn't able to give it much time, I will try to push it today by end of day (GMT+1).

vgvassilev · 2024-03-20T16:25:08Z

Is that work in the context of gsoc and if so, can you send me an email?

rohanjulka19 · 2024-03-20T16:41:52Z

I have sent you an email

github-actions

clang-tidy made some suggestions

include/clad/Differentiator/ReverseModeVisitor.h

github-actions · 2024-07-02T23:23:43Z

include/clad/Differentiator/ReverseModeVisitor.h

@@ -576,6 +575,8 @@
      clang::Expr* CreateCFTapePushExpr(std::size_t value);

    public:
+      const bool m_IsInvokedBySwitchStmt = false;


warning: member 'm_IsInvokedBySwitchStmt' of type 'const bool' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]

const bool m_IsInvokedBySwitchStmt = false; ^

github-actions · 2024-07-02T23:23:43Z

include/clad/Differentiator/ReverseModeVisitor.h

@@ -576,6 +575,8 @@
      clang::Expr* CreateCFTapePushExpr(std::size_t value);

    public:
+      const bool m_IsInvokedBySwitchStmt = false;


warning: member variable 'm_IsInvokedBySwitchStmt' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

const bool m_IsInvokedBySwitchStmt = false; ^

github-actions

clang-tidy made some suggestions

github-actions · 2024-07-03T10:20:43Z

include/clad/Differentiator/ReverseModeVisitor.h

@@ -576,6 +575,8 @@ namespace clad {
      clang::Expr* CreateCFTapePushExpr(std::size_t value);

    public:
+      bool m_IsInvokedBySwitchStmt = false;


warning: member variable 'm_IsInvokedBySwitchStmt' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

bool m_IsInvokedBySwitchStmt = false; ^

rohanjulka19 · 2024-07-03T12:10:46Z

Now in both forward and reverse pass, the condition check happens inside the loop body after the loop condition diff statements are executed. So now the order in which loop executes is -

Loop Increment
Loop Cond diff Stmt executes
Loop Cond Check and exit loop if check fails
Increment variable used track iteration count for reverse pass
Body Diff executes

This is done to ensure the condition diff executes one additional time when the loop condition fails right before the loop exits. Additionaly this handles the scenario where the forward pass loop terminates due to break stmt in that case in reverse pass for the first iteration loop cond diff is not executed.

parth-07

Thank you for the pull-request. Overall, the changes seem good. I will need some more time to review the pull-request in detail.

include/clad/Differentiator/ReverseModeVisitor.h

lib/Differentiator/ReverseModeVisitor.cpp

parth-07

The changes looks good. Can you please update the 'test/CUDA/GradientCuda.cu' test?

rohanjulka19 · 2024-07-14T21:02:36Z

The changes looks good. Can you please update the 'test/CUDA/GradientCuda.cu' test?

I have updated the Cuda test now it is passing. Can't seem to figure out why a test case is not working on x86, it's working on my laptop.

vgvassilev · 2024-07-15T09:43:03Z

@MihailMihov, this failure seems to be similar to what you've been hunting down?

MihailMihov · 2024-07-15T13:01:19Z

@MihailMihov, this failure seems to be similar to what you've been hunting down?

Seems like it's the same thing, in fact I'm not sure if one of the numbers isn't the exact same that I got somewhere. I'll see whether it happens on my machine too, like the other issue.

MihailMihov · 2024-07-15T22:15:39Z

Well I checked out your branch, but it seems like the issue is different. I had a really stupid error (uninitialized variable in my test). I ran your test with LLVM's MemorySanitizer and there is an uninitialized value here too, but it seems to be coming from one of the tapes.

==1626607==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55a8d77c8230 in main /home/mihail/dev/clad/test/Gradient/Loops.C:2666:3
    #1 0x7f3ac4a5f2df  (/usr/lib64/libc.so.6+0x262df)
    #2 0x7f3ac4a5f398 in __libc_start_main (/usr/lib64/libc.so.6+0x26398)
    #3 0x55a8d772b374 in _start (/home/mihail/dev/clad/build/ReverseLoops.out+0x32374)

  Uninitialized value was stored to memory at
    #0 0x55a8d780e329 in fn30_grad(double, double, double*, double*) /home/mihail/dev/clad/test/Gradient/../../include/clad/Differentiator/Tape.h
    #1 0x55a8d77c7c8f in fn30_grad(double, double, void*, void*) /home/mihail/dev/clad/test/Gradient/Loops.C:1:1
    #2 0x55a8d77c7c8f in _ZN4clad25execute_with_default_argsILb1EJEPFvddPvS1_EJiiRA5_dPdEJddS1_S1_ETnNSt9enable_ifIXT_EbE4typeELb1EEENS_15function_traitsIT1_E11return_typeENS_4listIJDpT0_EEESB_NSE_IJDpT3_EEEDpOT2_ /home/mihail/dev/clad/test/Gradient/../../include/clad/Differentiator/Differentiator.h:123:12
    #3 0x55a8d77c7c8f in void clad::CladFunction<void (*)(double, double, void*, void*), clad::NoObject, true>::execute_helper<void (*)(double, double, void*, void*), int, int, double (&) [5], double*>(void (*)(double, double, void*, void*), int&&, int&&, double (&) [5], double*&&) /home/mihail/dev/clad/test/Gradient/../../include/clad/Differentiator/Differentiator.h:267:14
    #4 0x55a8d77c7c8f in std::enable_if<!std::is_same<void (*)(double, double, void*, void*), clad::NoFunction*>::value, void>::type clad::CladFunction<void (*)(double, double, void*, void*), clad::NoObject, true>::execute<int, int, double (&) [5], double*, void (*)(double, double, void*, void*)>(int&&, int&&, double (&) [5], double*&&) /home/mihail/dev/clad/test/Gradient/../../include/clad/Differentiator/Differentiator.h:218:14
    #5 0x55a8d77c7c8f in main /home/mihail/dev/clad/test/Gradient/Loops.C:2666:3
    #6 0x7f3ac4a5f2df  (/usr/lib64/libc.so.6+0x262df)

  Uninitialized value was created
    <empty stack>

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/mihail/dev/clad/test/Gradient/Loops.C:2666:3 in main
Exiting

In order to see a more detailed stack trace I'd need to compile clad instrumented with MSan, but I couldn't figure it out today, I'll try to get you a better stack trace tomorrow.

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

rohanjulka19 · 2024-07-16T02:19:43Z

ok thanks to @MihailMihov, turns out there was a variable which wasn't initialised and was causing the test to fail. Now that it is initialised to zero, the test is passing. The variable is _d_cond which is added to ensure that while differentiating expression containing && operator, the expression which is then assigned to the cond variable gets differentiated.

cond = (c==0) && (res += i *j)

@MihailMihov Not sure if this is related to the warning you caught.

This change differentiates the loop condition expression. Additionaly if in forward pass a loop terminates pre-maturely due to break stmt. The reverse pass should start differentiation with break statment and not the loop condition differentiation. This change keeps track of whether the break was called in forward pass and based on that in reverse mode it is decided whether the loop differentiation is skipped for the first iteration or not.

MihailMihov · 2024-07-16T12:34:27Z

ok thanks to @MihailMihov, turns out there was a variable which wasn't initialised and was causing the test to fail. Now that it is initialised to zero, the test is passing. The variable is _d_cond which is added to ensure that while differentiating expression containing && operator, the expression which is then assigned to the cond variable gets differentiated.
cond = (c==0) && (res += i *j)
@MihailMihov Not sure if this is related to the warning you caught.

I just tried running the memory sanitizer with your new changes and the warning is gone, so that was likely the issue.

rohanjulka19 changed the title ~~Differentiate for loop condition~~ Differentiate for loop condition expression Mar 12, 2024

rohanjulka19 force-pushed the master branch from 0d37c3f to 0b673c9 Compare March 12, 2024 01:06

rohanjulka19 force-pushed the master branch from 0b673c9 to 1cdea66 Compare March 12, 2024 08:20

vgvassilev requested a review from parth-07 March 12, 2024 11:42

rohanjulka19 force-pushed the master branch from 1cdea66 to 5b23d64 Compare March 12, 2024 12:26

parth-07 requested changes Mar 12, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch from 5b23d64 to 442f39b Compare March 15, 2024 12:29

github-actions bot reviewed Mar 15, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch 2 times, most recently from 5801e5a to 728c53e Compare March 15, 2024 13:01

rohanjulka19 force-pushed the master branch from 728c53e to 709bc62 Compare March 15, 2024 13:30

rohanjulka19 requested a review from parth-07 March 15, 2024 13:34

parth-07 requested changes Mar 16, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch from adb5340 to 07cdac5 Compare July 2, 2024 23:16

github-actions bot reviewed Jul 2, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch from 07cdac5 to b6670d4 Compare July 3, 2024 10:13

github-actions bot reviewed Jul 3, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch 2 times, most recently from 2777d90 to e604ae7 Compare July 3, 2024 11:52

rohanjulka19 requested a review from parth-07 July 3, 2024 12:10

parth-07 reviewed Jul 8, 2024

View reviewed changes

parth-07 requested changes Jul 11, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch 3 times, most recently from 1f4a7e6 to ff1d72f Compare July 13, 2024 15:47

rohanjulka19 requested a review from parth-07 July 13, 2024 16:43

parth-07 reviewed Jul 14, 2024

View reviewed changes

rohanjulka19 force-pushed the master branch from ff1d72f to 99b910e Compare July 14, 2024 20:38

rohanjulka19 requested a review from parth-07 July 14, 2024 21:02

rohanjulka19 force-pushed the master branch from 99b910e to 05c947d Compare July 16, 2024 01:35

github-actions bot reviewed Jul 16, 2024

View reviewed changes

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

rohanjulka19 force-pushed the master branch from 05c947d to 59c7ef9 Compare July 16, 2024 01:47

rohanjulka19 force-pushed the master branch from 59c7ef9 to 3bd70e3 Compare July 16, 2024 11:29

parth-07 approved these changes Jul 17, 2024

View reviewed changes

vgvassilev merged commit e04d04e into vgvassilev:master Jul 18, 2024
89 checks passed

Differentiate for loop condition expression #818

Differentiate for loop condition expression #818

Conversation

rohanjulka19 commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

vgvassilev commented Mar 12, 2024

rohanjulka19 commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

parth-07 left a comment

Choose a reason for hiding this comment

rohanjulka19 commented Mar 13, 2024

rohanjulka19 commented Mar 13, 2024

parth-07 commented Mar 13, 2024

rohanjulka19 commented Mar 14, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Mar 15, 2024

Choose a reason for hiding this comment

github-actions bot Mar 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 15, 2024

rohanjulka19 commented Mar 15, 2024 • edited Loading

github-actions bot commented Mar 15, 2024

parth-07 left a comment • edited Loading

Choose a reason for hiding this comment

vgvassilev commented Mar 20, 2024

rohanjulka19 commented Mar 20, 2024

vgvassilev commented Mar 20, 2024

rohanjulka19 commented Mar 20, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 2, 2024

Choose a reason for hiding this comment

github-actions bot Jul 2, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 3, 2024

Choose a reason for hiding this comment

rohanjulka19 commented Jul 3, 2024

parth-07 left a comment

Choose a reason for hiding this comment

parth-07 left a comment

Choose a reason for hiding this comment

rohanjulka19 commented Jul 14, 2024 • edited Loading

vgvassilev commented Jul 15, 2024

MihailMihov commented Jul 15, 2024

MihailMihov commented Jul 15, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

rohanjulka19 commented Jul 16, 2024

MihailMihov commented Jul 16, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

rohanjulka19 commented Mar 14, 2024 •

edited

Loading

rohanjulka19 commented Mar 15, 2024 •

edited

Loading

parth-07 left a comment •

edited

Loading

rohanjulka19 commented Jul 14, 2024 •

edited

Loading