Add Support for Functions with Constant Array Type Parameters in Jacobian Mode #478
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @vgvassilev @sudo-panda for review |
Nirhar
commented
Aug 3, 2022
| @@ -38,6 +41,10 @@ namespace clad { | |||
| // several private/protected members of the visitor classes. | |||
| friend class ErrorEstimationHandler; | |||
| llvm::SmallVector<const clang::ValueDecl*, 16> m_IndependentVars; | |||
There was a problem hiding this comment.
It would have been a cleaner solution if this was a vector of strings storing the identifier names. Similarly, all unordered maps should have been maps from identifier names as strings to jacobian expressions rather than ValueDecl to jacobian expressions
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
==========================================
+ Coverage 92.72% 92.77% +0.05%
==========================================
Files 35 35
Lines 5454 5496 +42
==========================================
+ Hits 5057 5099 +42
Misses 397 397
|
vgvassilev
reviewed
Aug 11, 2022
| for (auto arg : args) { | ||
| // FIXME: fix when adding array inputs, now we are just skipping all | ||
| // array/pointer inputs (not treating them as independent variables). | ||
| if (utils::isArrayOrPointerType(arg->getType())) { | ||
| if (utils::isArrayOrPointerType(arg->getType())) { //is a array or pointer type parameter | ||
| if (arg->getName() == "p") |
There was a problem hiding this comment.
Why we have this special case here? Wouldn't it removing it will help?
There was a problem hiding this comment.
This special case was added by baidyanath in previous commits. He told me its something related to ROOT.
| @@ -25,6 +25,9 @@ namespace clad { | |||
| class ExternalRMVSource; | |||
| class MultiplexExternalRMVSource; | |||
|
|
|||
| using VectorOutputString = | |||
| std::vector<std::unordered_map<std::string, clang::Expr*>>; | |||
There was a problem hiding this comment.
Can you update the commit message and explain the overall problem and your solution. I feel that we can avoid these vectors of maps of strings.
Nirhar
force-pushed
the
jacobian_array_error
branch
from
161d295 to
fb567d0
Compare
…bian Mode
Now One must be able to find the Jacobian of functions with Constant Arrays in the parameter list.
For example, a function of the form:
```cpp
void func(double arr[3], double x, double y, double* output){
output[0]=arr[2]*x*y;
.
.
.
output[n-1]=arr[0]*arr[1]*arr[2];
}
```
will generate a Jacobian of size n x 5.
Corresponding tests for the same have been written.
Previously this feature was not implememnted because it was necessary to know the size
of an array to correctly determine the size of the output jacobian matrix. This has now
been achieved for constant arrays, as we know the array sizes for them and hence we can
precisely locate where each jacobian entry for a array parameter is located. This is
achieved with the help of m_IndependentVarsSize, which stores the number of actual parameters
that each function parameter corresponds to.
Prior to this commit a std::map<ValueDecl, Expr> was used to map variable declarations to
their corresponding jacobian expression, so that we can lookup the latter during the reversemode
computations quickly. While this is fine for primitive variables(because each variable will only
correspond to one jacobian expression), an array declaration will correspond to multiple jacobian
expressions, depending on which index of the array is being referred to. Since the ValueDecl can
only refer to the name of the array and not name+index, we must update the map to use the name of
the array + index as the key. This can be done easily by using a string representation of the
ValueDecl name with the index as a suffix as the key of the map. Hence variables like
m_ExprVariables, m_VectorOutputString have been introduced to map array declarations, indexed by
position in array to their corresponding jacobian expressions.
Closes vgvassilev#472
Nirhar
force-pushed
the
jacobian_array_error
branch
from
fb567d0 to
b8d0fb6
Compare
vgvassilev
reviewed
Aug 12, 2022
| m_IndependentVarsSize.push_back(sizeOfArray); | ||
|
|
||
| for (int j = 0; j < sizeOfArray; j++) { | ||
| std::string name = |
There was a problem hiding this comment.
I believe we can have a struct like:
struct IndexInfo {
ValueDecl* Arg;
unsigned Index;
};
Then we can replace the std::string in the maps and have efficient lookup based on the ValueDecl pointer and the index.
parth-07
reviewed
Oct 28, 2022
| m_Sema.CreateBuiltinArraySubscriptExpr(m_Result, noLoc, i, noLoc) | ||
| .get(); | ||
| m_Variables[arg] = result_at_i; | ||
| m_ExprVariables[arg->getNameAsString()] = result_at_i; |
There was a problem hiding this comment.
Why do we need m_ExprVariables? I do not see values being read from it anywhere.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now One must be able to find the Jacobian of functions with Constant Arrays in the parameter list.
For example, a function of the form:
will generate a Jacobian of size n x 5.
Corresponding tests for the same have been written.
Previously this feature was not implememnted because it was necessary to know the size
of an array to correctly determine the size of the output jacobian matrix. This has now
been achieved for constant arrays, as we know the array sizes for them and hence we can
precisely locate where each jacobian entry for a array parameter is located. This is
achieved with the help of m_IndependentVarsSize, which stores the number of actual parameters
that each function parameter corresponds to.
Prior to this commit a std::map<ValueDecl, Expr> was used to map variable declarations to
their corresponding jacobian expression, so that we can lookup the latter during the reversemode
computations quickly. While this is fine for primitive variables(because each variable will only
correspond to one jacobian expression), an array declaration will correspond to multiple jacobian
expressions, depending on which index of the array is being referred to. Since the ValueDecl can
only refer to the name of the array and not name+index, we must update the map to use the name of
the array + index as the key. This can be done easily by using a string representation of the
ValueDecl name with the index as a suffix as the key of the map. Hence variables like
m_ExprVariables, m_VectorOutputString have been introduced to map array declarations, indexed by
position in array to their corresponding jacobian expressions.
Closes #472