Fix for Parenthesis issue in gradients #1180
Conversation
| // Fixes consecutive negative signs | ||
| // (-1)*(-x) gives *_d_x += 1 * 1; instead of | ||
| // *_d_x += --1 * 1; | ||
| CUDA_HOST_DEVICE void ResolveParenthesis(char* temp, const char* code) { |
There was a problem hiding this comment.
We do not do textual parsing and transformations. The solution must be on AST level in one of our Visitor classes. You should check our documentation for more details.
| to.emplace_back(val); | ||
| return val; | ||
| } | ||
| /// Add value to the end of the tape, return the same value. |
There was a problem hiding this comment.
Running clang-format on the entire file is not a good idea. We should run it only on the changes with git-clang-format HEAD~.
|
@vgvassilev Thanks! I went through the documentation and have fixed the issue in the What should be done to deal with such instances or those 5 tests need to be changed ? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include <cstring> | ||
| #include <stddef.h> |
There was a problem hiding this comment.
warning: inclusion of deprecated C++ header 'stddef.h'; consider using 'cstddef' instead [modernize-deprecated-headers]
| #include <stddef.h> | |
| #include <cstddef> |
| // Fixes consecutive negative signs | ||
| // (-1)*(-x) gives *_d_x += 1 * 1; instead of | ||
| // *_d_x += --1 * 1; | ||
| CUDA_HOST_DEVICE void ResolveParenthesis(char* temp, const char* code) { |
There was a problem hiding this comment.
warning: function 'ResolveParenthesis' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
CUDA_HOST_DEVICE void ResolveParenthesis(char* temp, const char* code) {
^Additional context
include/clad/Differentiator/Differentiator.h:46: make as 'inline'
CUDA_HOST_DEVICE void ResolveParenthesis(char* temp, const char* code) {
^| if (*code == '-') { | ||
| current_block = 1; | ||
| negative_sign_count ^= 1; | ||
| code++; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
code++;
^| } | ||
| if (current_block && *code != ' ') { | ||
| if (negative_sign_count == 1) { | ||
| *temp++ = '-'; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
*temp++ = '-';
^| if (negative_sign_count == 1) { | ||
| *temp++ = '-'; | ||
| if (*code == '(') | ||
| *temp++ = ' '; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
*temp++ = ' ';
^| negative_sign_count = 0; | ||
| } | ||
| if (current_block && *code == ' ') { | ||
| code++; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
code++;
^| code++; | ||
| continue; | ||
| } | ||
| *temp++ = *code++; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
*temp++ = *code++;
^| code++; | ||
| continue; | ||
| } | ||
| *temp++ = *code++; |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
*temp++ = *code++;
^| /// N. | ||
| template <typename T> CUDA_HOST_DEVICE void zero_init(T* x, std::size_t N) { | ||
| for (std::size_t i = 0; i < N; ++i) | ||
| zero_init(x[i]); |
There was a problem hiding this comment.
warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
zero_init(x[i]);
^| /// Initialize a const sized array. | ||
| // NOLINTBEGIN(cppcoreguidelines-avoid-c-arrays) | ||
| template <typename T, std::size_t N> | ||
| CUDA_HOST_DEVICE void zero_init(T (&arr)[N]) { |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
CUDA_HOST_DEVICE void zero_init(T (&arr)[N]) {
^
toshit3q34
force-pushed
the
Fix_parenthesis_in_gradient
branch
from
7b900b8 to
9461931
Compare
lib/Differentiator/VisitorBase.cpp
Outdated
| return m_Sema.BuildUnaryOp(nullptr, OpLoc, OpCode, E).get(); | ||
| } | ||
| Expr* VisitorBase::RemoveFirstUnaryMinus(Expr* E, SourceLocation OpLoc) { | ||
| if (auto* UO = llvm::dyn_cast<UnaryOperator>(E)) { |
There was a problem hiding this comment.
warning: no header providing "llvm::dyn_cast" is directly included [misc-include-cleaner]
if (auto* UO = llvm::dyn_cast<UnaryOperator>(E)) {
^
lib/Differentiator/VisitorBase.cpp
Outdated
| return UO->getSubExpr(); | ||
| } | ||
| if (auto* BO = llvm::dyn_cast<BinaryOperator>(E)) { | ||
| if (BO->getOpcode() == BO_Mul || BO->getOpcode() == BO_Div) { |
There was a problem hiding this comment.
warning: no header providing "clang::BO_Div" is directly included [misc-include-cleaner]
if (BO->getOpcode() == BO_Mul || BO->getOpcode() == BO_Div) {
^
lib/Differentiator/VisitorBase.cpp
Outdated
| return UO->getSubExpr(); | ||
| } | ||
| if (auto* BO = llvm::dyn_cast<BinaryOperator>(E)) { | ||
| if (BO->getOpcode() == BO_Mul || BO->getOpcode() == BO_Div) { |
There was a problem hiding this comment.
warning: no header providing "clang::BO_Mul" is directly included [misc-include-cleaner]
if (BO->getOpcode() == BO_Mul || BO->getOpcode() == BO_Div) {
^
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1180 +/- ##
========================================
Coverage 94.56% 94.57%
========================================
Files 51 51
Lines 8960 8845 -115
========================================
- Hits 8473 8365 -108
+ Misses 487 480 -7
... and 20 files with indirect coverage changes
|
|
@vgvassilev Codecov test is failing because there is no test to check those lines. |
lib/Differentiator/VisitorBase.cpp
Outdated
| return m_Sema.BuildUnaryOp(nullptr, OpLoc, OpCode, E).get(); | ||
| } | ||
| Expr* VisitorBase::RemoveFirstUnaryMinus(Expr* E, SourceLocation OpLoc) { |
There was a problem hiding this comment.
Rather than removing the minus we should figure out where we synthesize the minus and we need parens.
There was a problem hiding this comment.
I tried to do that at first and was able to do it as well. However for some cases like :
f(x) = (-(-1)) * (-(-(x))) if we try to parenthesize, the output becomes (-(-(-(-1))*1)) which looks quite cluttered and redundant. Similar thing also happens when we try for higher powers like
f(x) = (-1) * (-x) * (-x) * (-x)
Would you recommend continuing to parenthesize in such cases, or would it be acceptable to simplify the representation for clarity?
There was a problem hiding this comment.
It's better to add parenthesis only at the places where it is necessary.
There was a problem hiding this comment.
Okay sure. Can you please specify the expected output of f_grad() for f(x) = (-(-1)) * (-(-(x))) so that I have a basis to work with and can modify accordingly. Thanks.
toshit3q34
force-pushed
the
Fix_parenthesis_in_gradient
branch
from
9461931 to
f086371
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@vgvassilev I hope you have reviewed the code. I have done the following two things :
The codecov tests fail because there are no tests which check either of the above conditions. |
|
Can you add a few tests demonstrating this pull request works? |
toshit3q34
force-pushed
the
Fix_parenthesis_in_gradient
branch
from
f086371 to
f26b5bf
Compare
|
@vgvassilev I have added a test file containing two tests which covers the above stated scenarios. You may review it now. |
|
clang-tidy review says "All clean, LGTM! 👍" |
This resolves the issue with two or more consecutive negative signs while dumping the code.
While copying the code to m_Code, changes include iterating the loop and checking when there are two or more consecutive negative signs and resolves them to be either '-' or ' ' depending on the parity.
Eg.
-(-(-1))*(x)
Before : _d_x += - - -1 * 1;
After : _d_x += -1 * 1;
The differentiate remains intact.
Fixes : #1098