-
Notifications
You must be signed in to change notification settings - Fork 156
Store adjoints as decls in m_Variables in the reverse mode #1644
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
|
|
||
| /// Map used to keep track of variable declarations and match them | ||
| /// with their derivatives. | ||
| std::unordered_map<const clang::ValueDecl*, clang::Expr*> m_Variables; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_Variables' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::unordered_map<const clang::ValueDecl*, clang::Expr*> m_Variables;
^| clang::Expr* m_RestoreTracker = nullptr; | ||
| /// Map used to keep track of variable declarations and match them | ||
| /// with their derivatives. | ||
| std::unordered_map<const clang::ValueDecl*, clang::VarDecl*> m_Variables; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_Variables' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::unordered_map<const clang::ValueDecl*, clang::VarDecl*> m_Variables;
^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vgvassilev Should we avoid protected members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably that's to late to consider... Let's ignore this and maybe adjust the config file.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Currently, we track the correspondance of the original variables and their adjoints in a map `m_Variables` as `x` --> `_d_x`. However, `x` is stored a decl and `_d_x` is stored as an expr. The latter results in us reusing the same `DeclRefExpr` of `_d_x`. Apart from that, it makes it hard to get the actual declaration of `_d_x`.
2224e12 to
2706d84
Compare
|
|
||
| /// Map used to keep track of variable declarations and match them | ||
| /// with their derivatives. | ||
| std::unordered_map<const clang::ValueDecl*, clang::Expr*> m_Variables; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this files used for? In principle we cannot reuse expressions (or decl-ref expressions) as clang builds a separate whenever they are needed. Can we drop this in favor of some utility function that builds a new expression?
Currently, we track the correspondance of the original variables and their adjoints in a map
m_Variablesasx-->_d_x. However,xis stored a decl and_d_xis stored as an expr. The latter results in us reusing the sameDeclRefExprof_d_x. Apart from that, it makes it hard to get the actual declaration of_d_x.