Handle variable declarations in conditions of if-statements (#865) #891
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #891 +/- ##
==========================================
+ Coverage 94.77% 94.83% +0.05%
==========================================
Files 52 52
Lines 7715 7719 +4
==========================================
+ Hits 7312 7320 +8
+ Misses 403 399 -4
|
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
How about "Handle variable declarations in conditions of if-statements", then explain what this patch does and finish with fixes:... We will need to update the PR description and title accordingly. |
gojakuch
changed the title
|
You need to update the commit message accordingly, too. |
|
clang-tidy review says "All clean, LGTM! 👍" |
gojakuch
force-pushed
the
issue-865
branch
2 times, most recently
from
86475d6 to
327a974
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
| // clad::push(..., _t); | ||
| // reverse: | ||
| // If we are inside for loop, condDiff will be stored in the following | ||
| // way: forward: _t = cond; if (_t) { ... } clad::push(..., _t); reverse: |
There was a problem hiding this comment.
Do we need this change? The old comments in multiple lines make more sense.
There was a problem hiding this comment.
it has been changed automatically by my clang format for some reason
There was a problem hiding this comment.
maybe changing it into a multiline comment style (/* ... */) should fix it.
| @@ -902,37 +898,21 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, | |||
| StmtDiff thenDiff = VisitBranch(If->getThen()); | |||
| StmtDiff elseDiff = VisitBranch(If->getElse()); | |||
|
|
|||
| // It is problematic to specify both condVarDecl and cond thorugh | |||
| // Sema::ActOnIfStmt, therefore we directly use the IfStmt constructor. | |||
There was a problem hiding this comment.
Do we need to delete this comment?
There was a problem hiding this comment.
condVarDecl is always set to nullptr here now, because it doesn't change anything, so I'm not sure if this is relevant. we generally don't need condVarDecl in the generated statement, only cond
There was a problem hiding this comment.
So, would it make sense to use ActOnIfStmt now?
There was a problem hiding this comment.
I think it's possible, but I'm not sure what benefit that would have.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Can we test the c++20 syntax: if (int a = 12; a < 0) {
} |
There was a problem hiding this comment.
Looks good to me.
This patch does not handle if-condition side-effects for all cases. The below example gives incorrect derivative:
#include "clad/Differentiator/Differentiator.h"
#include <iostream>
#define show(x) std::cout << #x <<": " << x << "\n";
double fn(double u, double v) {
double res = 0;
double foo = v;
res = foo;
res += foo;
if ((foo = u) > 10) {
res += foo * v;
}
return res + foo;
}
int main() {
auto fn_grad = clad::gradient(fn);
double u, v;
u = 3, v = 5;
double du, dv;
du = dv = 0;
fn_grad.execute(u, v, &du, &dv);
show(du);
show(dv);
}Output:
du: 0
dv: 3
Expected Output:
du: 1
dv: 2
This case can be handled separately in a different pull-request. Can you please create an issue for this example?
test/Gradient/Gradients.C
Outdated
| INIT_GRADIENT(fn_cond_decl); | ||
|
|
||
| INIT_GRADIENT(fn_cond_side_eff); | ||
| dx = 0; |
There was a problem hiding this comment.
You don't need to explicitly reset the value to 0. TEST_GRADIENT takes care of that.
There was a problem hiding this comment.
oh, I didn't know this, thanks
yeah I was afraid that it would be the case but the simple things I tested worked out. I'll open up an issue about it. at least some of them work now. |
this doesn't work out well unfortunately. but it seems fixable. |
fixed it |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
I do not see a test case. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
already added |
This patch handles variable declarations in conditions of if-statements in reverse mode differentiation. This is done by actually visiting the conditions instead of just cloning them inside the VisitIfStmt and by choosing the condition variable DeclStmt over cond when possible. This also helps to handle conditions that may affect derivatives as a side-effect. Also a couple of blocks have been added to wrap some statements and preserve the correct logic in the constructed derivatives and some unused code has been removed from the method. Fixes: vgvassilev#865
|
clang-tidy review says "All clean, LGTM! 👍" |
This patch handles variable declarations in conditions of if-statements in reverse mode differentiation. This is done by actually visiting the conditions instead of just cloning them inside the VisitIfStmt and by choosing the condition variable DeclStmt over cond when possible. This also helps to handle conditions that may affect derivatives as a side-effect. Also a couple of blocks have been added to wrap some statements and preserve the correct logic in the constructed derivatives and some unused code has been removed from the method.
Fixes: #865