update ad-level so it uses a circular flowgraph

[SteveBronder]

Summary

This is a fix for the ad-level optimization. There was a bug where the monotone framework was not continuously iterating in the minimum fixed point algorithm to find the set of variables that can be made into data. This led to the algorithm stopping too early and possibly producing non-compilable C++ such as for the following model

data {
    matrix[10, 10] X_data;
}

parameters {
    matrix[10, 10] X_p;
}

transformed parameters {
    matrix[10, 10] X_tp1 = exp(X_data);
    matrix[10, 10] X_tp2 = exp(X_tp1);
    X_tp1 = X_p;
}

In the current impl the ad level optimization will think X_tp1 is an autodiff type but X_tp2 is data. Since we can't assign autodiff variables to data variables this would throw a C++ compiler error.

By connecting the end nodes to the initial nodes the minimum fixed point algorithm will continously run until no changes to the program are made. Now the above program deduces that since X_tp1 must be an autodiff type X_tp2 must be as well.

In the test model you'll see that lazy code motion actually allows X_tp2 to still be data, but that's because exp(X_data) is stored in a local variable and the code becomes the equivalent of

matrix[10, 10] X_tp2 = exp(exp(X_data));

Submission Checklist

• Run unit tests
• Documentation
  • If a user-facing facing change was made, the documentation PR is here:
  • OR, no user-facing changes were made

Release notes

bug fix for ad-level optimization

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[SteveBronder]

@WardBrian fyi the plan is to merge this, then O1, then the NA filling fix (minus turning it on by default and just the fix) then the O1 PR

[SteveBronder]

@rybern fyi I set you as the reviewer since a lot of this is actually your code I just modified

[rybern]

Great, I'll have a look tomorrow

[rybern]

Looks good to me as long as those tests results are right, I didn't double check those.

One thing you could consider adding is an expect test showing that name collisions can happen with this approach. Maybe a program like

parameters {
  real x;
  real y;
}
model {
  if (x > 1) {
    int a = x;
    y ~ normal(a, 1);
  } else {
    int a = 1; // a is AD but doesn't need to be
    y ~ normal(a, 1);
  }
}

so we can see the progress when we/if we fix that

[rybern]

Tiny nit: I'd call this Circular_Fwd_Flowgraph to distinguish it

[SteveBronder]

Word!

[rybern]

Just to double check, looks like X_p is AD and X_tp1-X_tp7 are non-AD, is that right?

[SteveBronder]

X_tp1 is ad but the rest are not which, with lcm, is what we expect

[SteveBronder]

One thing you could consider adding is an expect test showing that name collisions can happen with this approach. Maybe a program like

Let's leave that till @VMatthijs puts in that PR

[SteveBronder]

Aight think we are good! @rybern Once test passes and you approve feel free to merge