Fix Incorrect derivative when loops contains continue #833

kchristin22 · 2024-03-20T21:22:55Z

For more details please see #710 (comment). This PR concerns only the fix of #710, so the only change needed is the one regarding the position of the step of the forward loop's variable in the reverse pass. Tests need to be updated accordingly if the change is approved.

Closes #710, closes #851.

github-actions · 2024-03-20T21:28:50Z

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

parth-07

Thank you for the pull-request. The solution seems good. Can you please add tests and update the pull-request description?

Also, can you please open an issue regarding fixing incorrect derivatives when loops contains break? You can use this test case:

#include "clad/Differentiator/Differentiator.h"
#include <iostream>

#define show(x) std::cout << #x << ": " << x << "\n";

double fn(double u, double v) {
    double res = 0;
    for (int i = 1; i < 3; i++) {
        res += i * u;
        if (i == 1)
            break;
    }
    return res;
}

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

parth-07 · 2024-03-25T16:10:10Z

lib/Differentiator/ReverseModeVisitor.cpp

+    activeBreakContHandler->EndCFSwitchStmtScope();
+    activeBreakContHandler->UpdateForwAndRevBlocks(bodyDiff);
+    PopBreakContStmtHandler();
+
    // Increment statement in the for-loop is only executed if the iteration


Please update this comment.

kchristin22 · 2024-03-25T16:43:15Z

I will update the tests, the description and the comment.

Also, can you please open an issue regarding fixing incorrect derivatives when loops contains break? You can use this test case:

Sure thing!

kchristin22 · 2024-03-25T18:15:38Z

Upon checking out the above example, the error occurs after the change of this PR right? I noticed that the decrement of i-- should be after the switch case when there's a break stmt as otherwise we "miss" a value of i. I will open a new PR for the new issue after this PR is merged.

parth-07 · 2024-03-25T18:56:13Z

the error occurs after the change of this PR right?

Yes, however, the break statement had incorrect behavior without this PR change as well. It's just that the particular example that I added happen to work for Clad on master.

I noticed that the decrement of i-- should be after the switch case when there's a break stmt as otherwise we "miss" a value of i.

In the case of a break statement, the last increment does not happen. So we need to skip the corresponding decrement as well when the break statement is hit.

kchristin22 · 2024-03-30T18:27:31Z

I have not forgotten this. I will update it in the upcoming week. Sorry for the delay.

github-actions · 2024-04-04T08:47:03Z

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

codecov · 2024-04-04T08:49:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.43%. Comparing base (b8de117) to head (1bae2cd).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   94.42%   94.43%   +0.01%     
==========================================
  Files          50       50              
  Lines        8729     8751      +22     
==========================================
+ Hits         8242     8264      +22     
  Misses        487      487

Files with missing lines	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	`97.41% <100.00%> (+0.09%)`	⬆️
lib/Differentiator/ReverseModeVisitor.cpp	`95.66% <100.00%> (+0.02%)`	⬆️

Files with missing lines	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	`97.41% <100.00%> (+0.09%)`	⬆️
lib/Differentiator/ReverseModeVisitor.cpp	`95.66% <100.00%> (+0.02%)`	⬆️

github-actions · 2024-04-04T09:03:33Z

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

github-actions · 2024-04-04T09:16:23Z

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

parth-07

Can you please add a test case where Clad was giving incorrect derivative result before?

github-actions · 2024-04-06T16:05:10Z

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

kchristin22 · 2024-04-06T16:09:08Z

@parth-07 I tried to add a new function in Loops.C, but even though I get the correct result when I compile locally this file and run it, somehow when I test using make check-clad, the result is expected to be 15.00 and if I change that to 15.00 it then says that the result of fn21 is expected to be 7.00 (which is correct). The fn21_grad function printed as a log due to the error also doesn't match the one that was checked and succeeded.

Update: the problem lies in TBR analysis, the increment is not included. I'm working on it.

github-actions

clang-tidy made some suggestions

include/clad/Differentiator/Differentiator.h

github-actions · 2024-09-08T16:56:56Z

lib/Differentiator/ReverseModeVisitor.cpp

+      CreateCFTapeSizeExprForCurrentCase() {
+    return m_RMV.BuildOp(BinaryOperatorKind::BO_NE, m_ControlFlowTape->Size(),
+                         ConstantFolder::synthesizeLiteral(
+                             m_RMV.m_Context.IntTy, m_RMV.m_Context, 0));


warning: argument comment missing for literal argument 'val' [bugprone-argument-comment]

Suggested change

m_RMV.m_Context.IntTy, m_RMV.m_Context, 0));

m_RMV.m_Context.IntTy, m_RMV.m_Context, /*val=*/0));

Maybe we should use the new ZeroInit interface here that you wrote?

Since this doesn't refer to an initialization, I think ConstantFolder::synthesizeLiteral is a better fit. getZeroInit calls this function internally either way.

parth-07

Can you please add tests as well?

vgvassilev · 2024-09-12T15:11:41Z

Can we undo the formatting changes which are not related to the changes of this pr?

kchristin22 · 2024-09-12T15:56:04Z

Can we undo the formatting changes which are not related to the changes of this pr?

Yes sorry about that, I was trying to fix it, cause my local version of git-clang-format produces different results than the CI. I think now it includes only the necessary changes more or less.

parth-07 · 2024-09-14T10:41:37Z

@kchristin22 Can you please add tests in the PR?

kchristin22 · 2024-09-08T17:12:51Z

test/Gradient/Loops.C

@@ -2081,19 +2086,19 @@ double fn26(double i, double j) {
 // CHECK-NEXT:             if (!_t0)
 // CHECK-NEXT:                 break;
 // CHECK-NEXT:         }
+// CHECK-NEXT:         {


@PetroZarytskyi @ovdiiuv without using TBR, this follows the pattern of avoiding performing the increment differentiation step when this is the case of the break branch. So, this is derived like so when TBR analysis is not performed:

if (clad::size(_t3) != 0 && clad::back(_t3) != 1) { res = clad::pop(_t2); double _r_d1 = _d_res; _d_res = 0.; *_d_i += 7 * _r_d1 * j; *_d_j += 7 * i * _r_d1; _d_c += 0; --c; }

It seems that when the increment's diff is a compound stmt this is instead:

{ res = clad::pop(_t2); double _r_d1 = _d_res; _d_res = 0.; *_d_i += 7 * _r_d1 * j; *_d_j += 7 * i * _r_d1; _d_c += 0; --c; }

Is this intentional? Should I open an issue for this?

kchristin22 · 2024-09-12T10:18:05Z

lib/Differentiator/ReverseModeVisitor.cpp

+      CreateCFTapeSizeExprForCurrentCase() {
+    return m_RMV.BuildOp(BinaryOperatorKind::BO_NE, m_ControlFlowTape->Size(),
+                         ConstantFolder::synthesizeLiteral(
+                             m_RMV.m_Context.IntTy, m_RMV.m_Context, 0));


Since this doesn't refer to an initialization, I think ConstantFolder::synthesizeLiteral is a better fit. getZeroInit calls this function internally either way.

test/Gradient/Loops.C

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-27T08:09:30Z

lib/Differentiator/ReverseModeVisitor.cpp

+        m_CurrentBreakFlagExpr =
+            BuildOp(BinaryOperatorKind::BO_LOr,
+                    BuildOp(BinaryOperatorKind::BO_NE, revCounter,
+                            BuildDeclRef(loopCounter.getNumRevIterations())),


warning: initializing non-owner 'DefaultStmt *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]

auto* newDefaultStmt = ^

test/Gradient/Loops.C

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-27T17:34:17Z

lib/Differentiator/ReverseModeVisitor.cpp

-    // Increment statement in the for-loop is only executed if the iteration
-    // did not end with a break/continue statement. Therefore, forLoopIncDiff
-    // should be inside the last switch case in the reverse pass.
+    activeBreakContHandler->EndCFSwitchStmtScope();


warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]

Suggested change

activeBreakContHandler->EndCFSwitchStmtScope();

auto* forwardSS =

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-28T08:22:41Z

include/clad/Differentiator/ReverseModeVisitor.h

+      }
+
+      /// Returns the number of reverse iterations to be executed.
+      clang::VarDecl* getNumRevIterations() const { return m_numRevIterations; }


warning: function 'getNumRevIterations' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change

clang::VarDecl* getNumRevIterations() const { return m_numRevIterations; }

[[nodiscard]] clang::VarDecl* getNumRevIterations() const { return m_numRevIterations; }

parth-07

Looks good.

… pass

… tape size check

parth-07 requested changes Mar 25, 2024

View reviewed changes

kchristin22 mentioned this pull request Apr 4, 2024

Don't increment the for loop variable when there's a break statement #851

Closed

vgvassilev requested a review from parth-07 April 4, 2024 13:40

parth-07 requested changes Apr 4, 2024

View reviewed changes

github-actions bot reviewed Sep 8, 2024

View reviewed changes

vgvassilev requested review from PetroZarytskyi and parth-07 September 9, 2024 06:20

parth-07 requested changes Sep 11, 2024

View reviewed changes

kchristin22 force-pushed the 710-fix-loop-continue branch from 56b0c9f to 6f8bc21 Compare September 12, 2024 15:11

kchristin22 force-pushed the 710-fix-loop-continue branch from 70e3cc5 to 6f8bc21 Compare September 12, 2024 15:53

kchristin22 requested a review from parth-07 September 14, 2024 13:59

kchristin22 force-pushed the 710-fix-loop-continue branch from 0622a9b to b4917c0 Compare October 23, 2024 10:30

kchristin22 mentioned this pull request Oct 23, 2024

If stmt of break cond is not added before compound for-loop inc diff when using TBR analysis in reverse mode #1123

Open

kchristin22 commented Oct 23, 2024

View reviewed changes

parth-07 requested changes Oct 24, 2024

View reviewed changes

test/Gradient/Loops.C Outdated Show resolved Hide resolved

kchristin22 requested a review from parth-07 October 25, 2024 13:34

github-actions bot reviewed Oct 27, 2024

View reviewed changes

parth-07 requested changes Oct 27, 2024

View reviewed changes

test/Gradient/Loops.C Outdated Show resolved Hide resolved

github-actions bot reviewed Oct 27, 2024

View reviewed changes

kchristin22 force-pushed the 710-fix-loop-continue branch from e1745be to 348b3d2 Compare October 28, 2024 08:12

github-actions bot reviewed Oct 28, 2024

View reviewed changes

kchristin22 requested review from parth-07 and vgvassilev October 28, 2024 10:08

parth-07 approved these changes Oct 29, 2024

View reviewed changes

kchristin22 added 11 commits October 29, 2024 08:44

Add original loop variable's step prior to switch in reverse pass

3a7b269

Update comment on step increment

6d6ee85

Fix format conflicts

d713ef4

Revert formatting changes by running git clang-format locally

8cd2396

Add test cases of issues 710 and 851

cd4f32b

Fix size_t values for Arch tests

71dfc93

Fix unsigned var declarations for Arch tests

6582cb1

Avoid duplicate of tape size checking when there's a break stmt

906814c

Optimize check of break branch using first iteration check in reverse…

64493d1

… pass

Improve set function of numRevIterations

9ffefc1

Use different break cond for each loop and remove the now unnecessary…

1bae2cd

… tape size check

vgvassilev force-pushed the 710-fix-loop-continue branch from 05da570 to 1bae2cd Compare October 29, 2024 06:44

vgvassilev merged commit 21e4b4f into vgvassilev:master Oct 30, 2024
90 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Incorrect derivative when loops contains continue #833

Fix Incorrect derivative when loops contains continue #833

kchristin22 commented Mar 20, 2024 •

edited

Loading

github-actions bot commented Mar 20, 2024

parth-07 left a comment

parth-07 Mar 25, 2024

kchristin22 commented Mar 25, 2024

kchristin22 commented Mar 25, 2024 •

edited

Loading

parth-07 commented Mar 25, 2024

kchristin22 commented Mar 30, 2024

github-actions bot commented Apr 4, 2024

codecov bot commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024

github-actions bot commented Apr 4, 2024

parth-07 left a comment

github-actions bot commented Apr 6, 2024

kchristin22 commented Apr 6, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot Sep 8, 2024

vgvassilev Sep 9, 2024

kchristin22 Sep 12, 2024

parth-07 left a comment

vgvassilev commented Sep 12, 2024

kchristin22 commented Sep 12, 2024

parth-07 commented Sep 14, 2024

kchristin22 Sep 8, 2024

kchristin22 Sep 12, 2024

github-actions bot left a comment

github-actions bot Oct 27, 2024

github-actions bot left a comment

github-actions bot Oct 27, 2024

github-actions bot left a comment

github-actions bot Oct 28, 2024

parth-07 left a comment

	m_RMV.m_Context.IntTy, m_RMV.m_Context, 0));
	m_RMV.m_Context.IntTy, m_RMV.m_Context, /val=/0));

	activeBreakContHandler->EndCFSwitchStmtScope();
	auto* forwardSS =

	clang::VarDecl* getNumRevIterations() const { return m_numRevIterations; }
	[[nodiscard]] clang::VarDecl* getNumRevIterations() const { return m_numRevIterations; }

Fix Incorrect derivative when loops contains continue #833

Fix Incorrect derivative when loops contains continue #833

Conversation

kchristin22 commented Mar 20, 2024 • edited Loading

github-actions bot commented Mar 20, 2024

parth-07 left a comment

Choose a reason for hiding this comment

parth-07 Mar 25, 2024

Choose a reason for hiding this comment

kchristin22 commented Mar 25, 2024

kchristin22 commented Mar 25, 2024 • edited Loading

parth-07 commented Mar 25, 2024

kchristin22 commented Mar 30, 2024

github-actions bot commented Apr 4, 2024

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

github-actions bot commented Apr 4, 2024

github-actions bot commented Apr 4, 2024

parth-07 left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 6, 2024

kchristin22 commented Apr 6, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 8, 2024

Choose a reason for hiding this comment

vgvassilev Sep 9, 2024

Choose a reason for hiding this comment

kchristin22 Sep 12, 2024

Choose a reason for hiding this comment

parth-07 left a comment

Choose a reason for hiding this comment

vgvassilev commented Sep 12, 2024

kchristin22 commented Sep 12, 2024

parth-07 commented Sep 14, 2024

kchristin22 Sep 8, 2024

Choose a reason for hiding this comment

kchristin22 Sep 12, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 27, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 27, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 28, 2024

Choose a reason for hiding this comment

parth-07 left a comment

Choose a reason for hiding this comment

kchristin22 commented Mar 20, 2024 •

edited

Loading

kchristin22 commented Mar 25, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

kchristin22 commented Apr 6, 2024 •

edited

Loading