Skip to content
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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

PetroZarytskyi
Copy link
Collaborator

@PetroZarytskyi PetroZarytskyi commented Apr 15, 2024

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

double f(double x) {
    int n = x;
    return n;
}

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:

void f_grad(double x, double *_d_x) {
    int _d_n = 0;
    int n = x;
    goto _label0;
  _label0:
    _d_n += 1;
    *_d_x += _d_n;
}

this PR:

void f_grad(double x, double *_d_x) {
    int n = x;
    goto _label0;
  _label0:
    ;
}

Copy link
Contributor

@github-actions github-actions bot left a 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

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
llvm::SmallVectorImpl<Stmt*>* block /*=nullptr*/) {
DeclContext* originalFnDC = nullptr;
if (originalFD)
originalFnDC = const_cast<DeclContext*>(originalFD->getDeclContext());
Copy link
Contributor

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());
                     ^

lib/Differentiator/VisitorBase.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a 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

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a 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

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/VisitorBase.cpp Outdated Show resolved Hide resolved
lib/Differentiator/VisitorBase.cpp Outdated Show resolved Hide resolved
@PetroZarytskyi PetroZarytskyi force-pushed the int-diff branch 5 times, most recently from 20b6dfc to 2864c56 Compare April 30, 2024 09:51
Copy link
Contributor

@github-actions github-actions bot left a 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

lib/Differentiator/VisitorBase.cpp Outdated Show resolved Hide resolved
@PetroZarytskyi PetroZarytskyi force-pushed the int-diff branch 8 times, most recently from 433363d to 63d07c5 Compare April 30, 2024 16:55
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 97.36842% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 94.58%. Comparing base (d1fec23) to head (63d07c5).

Current head 63d07c5 differs from pull request most recent head 6183d10

Please upload reports for the commit 6183d10 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
...clude/clad/Differentiator/BaseForwardModeVisitor.h 100.00% <ø> (ø)
include/clad/Differentiator/CladUtils.h 100.00% <ø> (ø)
include/clad/Differentiator/DerivativeBuilder.h 100.00% <ø> (ø)
include/clad/Differentiator/Differentiator.h 100.00% <ø> (ø)
include/clad/Differentiator/ReverseModeVisitor.h 97.77% <100.00%> (-0.10%) ⬇️
include/clad/Differentiator/VisitorBase.h 100.00% <ø> (ø)
lib/Differentiator/CladUtils.cpp 93.58% <100.00%> (+0.81%) ⬆️
lib/Differentiator/DerivativeBuilder.cpp 100.00% <ø> (+0.99%) ⬆️
lib/Differentiator/ErrorEstimator.cpp 97.09% <100.00%> (-2.07%) ⬇️
lib/Differentiator/BaseForwardModeVisitor.cpp 98.71% <94.73%> (-0.20%) ⬇️
... and 2 more

... and 10 files with indirect coverage changes

Files Coverage Δ
...clude/clad/Differentiator/BaseForwardModeVisitor.h 100.00% <ø> (ø)
include/clad/Differentiator/CladUtils.h 100.00% <ø> (ø)
include/clad/Differentiator/DerivativeBuilder.h 100.00% <ø> (ø)
include/clad/Differentiator/Differentiator.h 100.00% <ø> (ø)
include/clad/Differentiator/ReverseModeVisitor.h 97.77% <100.00%> (-0.10%) ⬇️
include/clad/Differentiator/VisitorBase.h 100.00% <ø> (ø)
lib/Differentiator/CladUtils.cpp 93.58% <100.00%> (+0.81%) ⬆️
lib/Differentiator/DerivativeBuilder.cpp 100.00% <ø> (+0.99%) ⬆️
lib/Differentiator/ErrorEstimator.cpp 97.09% <100.00%> (-2.07%) ⬇️
lib/Differentiator/BaseForwardModeVisitor.cpp 98.71% <94.73%> (-0.20%) ⬇️
... and 2 more

... and 10 files with indirect coverage changes

@PetroZarytskyi PetroZarytskyi marked this pull request as ready for review May 1, 2024 11:29
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev
Copy link
Owner

Since this is largely backward incompatible change, can you open this PR against the ver-2.0 branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants