-
Notifications
You must be signed in to change notification settings - Fork 836
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
[WIP] Tag / debug tape recordings #2343
base: develop
Are you sure you want to change the base?
Conversation
@@ -744,7 +744,7 @@ | |||
if (kind_recording == RECORDING::SOLUTION_VARIABLES) { | |||
cout << " Objective function : " << ObjFunc; | |||
if (driver_config->GetWrt_AD_Statistics()){ | |||
cout << " (" << ObjFunc_Index << ")\n"; | |||
// cout << " (" << ObjFunc_Index << ")\n"; |
Check notice
Code scanning / CodeQL
Commented-out code Note
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.
This will be resolved inside CoDiPack :)
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.
Looks good, can you document how it would be used?
…zation.hpp (to cover RealReverseTag).
Sure. Added an option so that you can compile the AD build using the tag type ( |
static constexpr bool StoreAsRef = true; | ||
|
||
private: | ||
alignas(Size * sizeof(Scalar)) Scalar x_[N]; | ||
alignas(Size * 32) Scalar x_[N]; |
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.
Would you be okay with replacing 32
by a function to find the next power of 2? (Requirement of alignas.) Or could this be done any better?
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.
For what type is the result not a power of 2?
If we use a fixed number instead of sizeof, arrays of this type may take more space than necessary.
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.
The reverse tag type is 24 I think. If sizeof(Scalar)
is already a power of 2 that function should of course not change it :)
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.
I see, so the tag doesn't have a float/double part? Then it should be ok to round Size * sizeof(Scalar) to the next power of 2.
Or maybe using Size * alignof(Scalar), that may be more generic and achieve the desired result.
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.
Well it does contain at least a double for the primal value (show must go on). I'll check or ask what the remaining bytes are for besides two int
s for index and tag. But sizeof(RealReverseTag) = 24
.
Proposed Changes
Add feature to record a tape for debug purposes (as a tool for AD / discrete adjoint / .. developers), using CoDiPack's
codi::RealReverseTag
.Related Work
Supposed to help finding remaining errors in #2317.
PR Checklist
Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.
pre-commit run --all
to format old commits.