Don't pass derived args to forw pass unless they are references #1207
Conversation
|
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 #1207 +/- ##
==========================================
- Coverage 94.75% 94.62% -0.13%
==========================================
Files 51 51
Lines 8883 8877 -6
==========================================
- Hits 8417 8400 -17
- Misses 466 477 +11
... and 1 file with indirect coverage changes
|
PetroZarytskyi
force-pushed
the
forw-arg
branch
from
5c77547 to
fcd60fd
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| // CHECK-NEXT: std::vector<double> _t15 = vec; | ||
| // CHECK-NEXT: {{.*}}ValueAndAdjoint<double &, double &> _t16 = {{.*}}class_functions::operator_subscript_reverse_forw(&vec, 2, &_d_vec, _r6); | ||
| // CHECK-NEXT: {{.*}}ValueAndAdjoint<double &, double &> _t16 = {{.*}}class_functions::operator_subscript_reverse_forw(&vec, 2, &_d_vec, {{0U|0UL|0}}); |
There was a problem hiding this comment.
Writing FileCheck CHECKs that satisfy all compiler/runtime combinations seems can get very time-consuming. @vgvassilev @PetroZarytskyi What do you think about testing the FileCheck CHECKs for just one compiler/runtime?
There was a problem hiding this comment.
Yes, alternatively we can just put auto for the ValueAndAdjoint. I do not think we will need a lot of regex magic after we landed #1171.
PetroZarytskyi
force-pushed
the
forw-arg
branch
from
fcd60fd to
483b0ce
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
@vgvassilev The coverage decreased because we no longer visit |
Understood I can merge. |
…inters. ``_reverse_forward`` functions only contain the forward pass, which only affects the derivatives on references/pointers. For other types, there is no point in passing the adjoint. Moreover, doing so is often incorrect because derived arguments are generated for the reverse pass, e.g. ``` // forward pass operator_subscript_reverse_forw(&vec, 0, &_d_vec, _r0); // `_r0` is declared later ... // reverse pass size_type _r0 = 0UL; operator_subscript_pullback(&_t2, 0, 1, &_d_vec, &_r0); ``` In this example, replacing the first occurrence of _r0 with 0 will still be correct.
|
clang-tidy review says "All clean, LGTM! 👍" |
_reverse_forwardfunctions only contain the forward pass, which only affects the derivatives on references/pointers. For other types, there is no point in passing the adjoint. Moreover, doing so is often incorrect because derived arguments are generated for the reverse pass, e.g.In this example, replacing the first occurrence of _r0 with 0 will still be correct.