-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't create adjoints for integral type variables #864
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
llvm::SmallVectorImpl<Stmt*>* block /*=nullptr*/) { | ||
DeclContext* originalFnDC = nullptr; | ||
if (originalFD) | ||
originalFnDC = const_cast<DeclContext*>(originalFD->getDeclContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
originalFnDC = const_cast<DeclContext*>(originalFD->getDeclContext());
^
c5194e2
to
c993a64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
dc21b9f
to
9e343b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
20b6dfc
to
2864c56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
433363d
to
63d07c5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #864 +/- ##
==========================================
- Coverage 94.82% 94.58% -0.25%
==========================================
Files 52 51 -1
Lines 7708 7678 -30
==========================================
- Hits 7309 7262 -47
- Misses 399 416 +17
... and 10 files with indirect coverage changes
|
… start using overloads in error estimation
clang-tidy review says "All clean, LGTM! 👍" |
Since this is largely backward incompatible change, can you open this PR against the |
This PR removes adjoints for integral types in the reverse mode. This is done because integers are not differentiable in the traditional mathematical sense. Such behavior is consistent with other AD tools like Tapenade and Enzyme. Let's consider an example to see the difference in behavior
This is essentially a floor function so mathematically the derivative should be 0. However, Clad master produces 1. Let's compare the derivatives on master and in this PR.
master:
this PR: