Fix custom reverse_forws for operators
#1076
Conversation
Previously, nullptr used to be set as the derivative of a call in the reverse mode, if there was a custom reverse_forw function available. This issue was overlooked at first, since it doesn't cause any trouble, unless someone decides to use nested operators (such as expressions of the form `a[i] = x*x`). Fixes: vgvassilev#1070
gojakuch
force-pushed
the
STL-reverse
branch
from
11443bf to
c2f0670
Compare
|
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 #1076 +/- ##
=======================================
Coverage 94.41% 94.41%
=======================================
Files 55 55
Lines 8410 8410
=======================================
Hits 7940 7940
Misses 470 470
|
| // CHECK-NEXT: double _t6 = _t5.value; | ||
| // CHECK-NEXT: _t5.value = x * x; | ||
| // CHECK-NEXT: std::vector<double> _t7 = a; | ||
| // CHECK-NEXT: clad::ValueAndAdjoint<double &, double &> _t8 = {{.*}}operator_subscript_reverse_forw(&a, 1, &_d_a, _r1); |
There was a problem hiding this comment.
nullptr was being passed to reverse_forws intentionally. We cannot pass adjoints uniformally for non-reference/non-pointer cases. Now, the generated code cannot be compiled because _r1 is being passed to the reverse_forw function call even though it is defined later in the reverse-pass.
There was a problem hiding this comment.
I know, I've opened an issue about this (#1066) this week and I think it was partially introduced in the original PR about custom reverse_forws for constructors, so I didn't really register this as an issue. it's still better this way because it at least has the correct logic now. but such misuses of yet-to-be-declared variables are happening all over in the reverse mode (at least that's what I thought), so I didn't pay that much attention to this. I'll add this as a note to #1066 or open a separate issue about this. thanks for your comment, cos I'd have forgotten to track this otherwise.
There was a problem hiding this comment.
it's still better this way because it at least has the correct logic now.
I don't think it is the correct logic now. The reverse_forw call now refers to a variable that is defined much later, that does not seem like a correct logic to me.
but such misuses of yet-to-be-declared variables are happening all over in the reverse mode (at least that's what I thought)
Please correct me if I am wrong, I think @PetroZarytskyi made significant efforts to reduce such cases. We certainly should not knowingly add more such cases.
I'll add this as a note to #1066 or open a separate issue about this. thanks for your comment, cos I'd have forgotten to track this otherwise.
I think passing nullptr where adjoint is unavailable in the forward-pass is the right way to go. @vgvassilev What do you recommend here?
There was a problem hiding this comment.
I think I see both points. @gojakuch is unblocked by this change on the one hand and on the other it is problematic. Do we have an alternative way to fix his example cases where the nullptr approach breaks?
There was a problem hiding this comment.
Do we have an alternative way to fix his example cases where the nullptr approach breaks?
I opened #1079 yesterday to track this. I think there should be a way, I'll get back to this.
Previously, nullptr used to be set as the derivative of a call in the reverse mode, if there was a custom reverse_forw function available. This issue was overlooked at first, since it doesn't cause any trouble, unless someone decides to use nested operators (such as expressions of the form
a[i] = x*x).Fixes: #1070