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

Regression with Clad master in case of num-diff fallback in specific conditions #681

Closed
guitargeek opened this issue Dec 13, 2023 · 6 comments · Fixed by #738
Closed

Regression with Clad master in case of num-diff fallback in specific conditions #681

guitargeek opened this issue Dec 13, 2023 · 6 comments · Fixed by #738
Assignees
Milestone

Comments

@guitargeek
Copy link
Contributor

Some of the RooFit tests are still failing with Clad master, because of code that hits very specific conditions.

To reproduce the underlying problem, first create a library with any function:

// clang++ -fPIC mylib.cxx -c -o mylib.o && ar r libmylib.a mylib.o

double foo(double x, double y, double z)
{
   return x * y * z;
}

Then use that library in a function the is differentiated:

// Compile with clang++ -O2 -std=c++20 -fplugin=/usr/lib/clad.so repro_regression.cxx -o repro_regression -lmylib -L.

#include "clad/Differentiator/Differentiator.h"

#include <iostream>
#include <vector>

double foo(double x, double y, double z);

double wrapper(double *params)
{
   if (true) {
      double constant = 11.;
      return foo(params[0], constant, 1.);
   }

   return 0.0;
}

int main()
{
   std::vector<double> params{4};

   std::cout << "Value" << std::endl;
   const double nllVal = wrapper(params.data());
   std::cout << nllVal << std::endl;
   std::cout << std::endl;

   auto grad = clad::gradient(wrapper, "params");

   std::vector<double> gradResult(1);
   grad.execute(params.data(), gradResult.data());

   std::cout << "Clad diff:" << std::endl;
   std::cout << gradResult[0] << std::endl;
   std::cout << std::endl;

   auto numDiff = [&](int i) {
      const double eps = 1e-6;
      std::vector<double> p{params};
      p[i] = params[i] - eps;
      double nllValDown = wrapper(p.data());
      p[i] = params[i] + eps;
      double nllValUp = wrapper(p.data());
      return (nllValUp - nllValDown) / (2 * eps);
   };

   std::cout << "Num diff:" << std::endl;
   std::cout << numDiff(0) << std::endl;
}

Many conditions need to be met to get this Clad error:

  • You need to compile with -O2
  • You need to call a function in a library such that Clad falls back to num. diff.
  • The code needs to be in a branch, like this if (true) in the reproducer
  • One of the function variables must be a named variable (i.e., doing foo(params[0], 11., 1.) would not hit the bug)

The output will be:

Value
44

Clad diff:
0

Num diff:
11

My compiler setup:

clang version 16.0.6
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /bin
@guitargeek
Copy link
Contributor Author

guitargeek commented Jan 22, 2024

While the reproducer in the original post works with the fix by @PetroZarytskyi, the underlying issue doesn't seem to really fixed because adding another trivial if statement after defining the constant breaks Clad again:

double wrapper(double *params)
{
   if (true) {
      double constant = 11.;
      if (true) { // NEW
         return foo(params[0], constant, 1.);
      } // NEW
   }

   return 0.0;
}

@vgvassilev
Copy link
Owner

@guitargeek, can you provide us with the values of params and the expected result of wrapper?

@guitargeek
Copy link
Contributor Author

You can just take the code from the initial post in this issue, and replace the wrapper there with the new one. The reproducer provides the params values and computes the expected gradient numerically

@vgvassilev
Copy link
Owner

Do we care what foo does?

@guitargeek
Copy link
Contributor Author

Mathematically no, the only requirement to trigger the problem is that de definition of foo is not known to Clad. That's why I put it into a separate shared library in the reproducer

@guitargeek
Copy link
Contributor Author

Thanks for fixing things up to this point! The current failures with RooFit can be reduced to this function, which can be plugged in to the full reproducer at the beginning of the issue:

double wrapper(double *params)
{
   double obs[]{
      110,
   };

   double result = 0.0;
   double hist[1] = {50.};
   double tmp6[] = {params[0], 1.};

   for (int j = 0; j < 1; j++) {
      double signalshapes = hist[j];
      double background1shapes = hist[j];
      double tmp0[] = {signalshapes, background1shapes};
      double tmp9 = 0.;
      for (int i = 0; i < 2; i++) {
         tmp9 += tmp0[i] * tmp6[i];
      }
      result += std::log(tmp9) - std::lgamma(obs[j] + 1.);
   }

   return result;
}

It seems to be related to double-loops again

vgvassilev pushed a commit to PetroZarytskyi/clad that referenced this issue Feb 8, 2024
When we produce a gradient function we generally have a forward and reverse
sweep. In the forward sweep we accumulate the state and in the reverse sweep
we use that state to invert the program execution. The forward sweep generally
follows the sematics of the primal function and when neccessary stores the state
which would be needed but lost for the reverse sweep.

However, to minimize the stores onto the tape we need to reuse some of the
variables between the forward and the reverse sweeps which requires some
variables to be promoted to the enclosing lexical scope of both sweeps.

Fixes vgvassilev#659, fixes vgvassilev#681.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants