Fix errors due to recursive calling of HandleTopLevelDecl #897
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #897 +/- ##
==========================================
- Coverage 94.82% 94.15% -0.67%
==========================================
Files 52 53 +1
Lines 7708 7803 +95
==========================================
+ Hits 7309 7347 +38
- Misses 399 456 +57
... and 2 files with indirect coverage changes
|
|
To test this locally with Clang, we can declare a custom derivative (for a called function) but cannot offer its definition. Now, computing the hessian should result in a linker error instead of a compile-time error. Minimal code: #include "clad/Differentiator/Differentiator.h"
namespace clad{
namespace custom_derivatives {
clad::ValueAndPushforward<double, double> f_pushforward(double x, double _d_x);
}
}
double f(double x) {
return x*x;
}
double wrapper(double x) {
return f(x);
}
int main() {
clad::hessian(wrapper);
return 0;
} |
|
Can we add this test to the PR? |
|
As mentioned above, the crude way to test this locally was seeing a linker error instead of a compile time error because of numerical differentiation. I don't think it would be good to have something like that in the CI check. We can test this with ROOT + Cling infrastructure manually though. |
|
@vaithak We can test this by adding multiple files for the test, right? For example, we can have two files, |
That would be probably better in the unittests. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
changed the title
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I am wondering why the coverage was decreased when we added a test… quite strange. Can you fix the failures and squash into a single commit? |
|
clang-tidy review says "All clean, LGTM! 👍" |
I need to fix Kokkos test, not sure what is happening there. |
The kokkos failure looks like concurrency failure. We built this file before building clad.so? |
- Add custom derivative to DerivativeSet: This is required if the definition of the custom derivative is not found in the current translation unit and is linked in from another. Adding it to the set of derivatives ensures that the custom derivative is not differentiated again using numerical differentiation due to an unavailable definition. - Fix recursive processing of DiffRequests: There can be cases where `m_Multiplexer` is not provided. Hence, we don't delay HandleTranslationUnit at the end and it is called repeatedly. This resulted in HandleTopLevelDecl being called recursively (from PerformPendingInstantiations). This commit adds conditional checks to ensure this doesn't perturb the execution of the differentiation plan.
|
clang-tidy review says "All clean, LGTM! 👍" |
Adding it to the set of derivatives ensures that the custom derivative is not differentiated again using numerical differentiation due to an unavailable definition.
m_Multiplexeris not provided. Hence, we don't delay HandleTranslationUnit at the end and it is called repeatedly. This resulted in HandleTopLevelDecl being called recursively (from PerformPendingInstantiations). This PR adds conditional checks to ensure this doesn't perturb the execution of the differentiation plan.Fixes: #890