-
Notifications
You must be signed in to change notification settings - Fork 156
Add primitive docs for custom derivatives in Clad sphinx docs #1444
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
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
vgvassilev
left a comment
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 looks quite promising.
@PetroZarytskyi, can you take a look and possibly review this PR, too.
@davidlange6, can you check English?
| Note that the constructor pullback does not need anything such as | ||
| :code:`clad::ConstructorPushforwardTag<::Coordinates>`. It is because | ||
| the constructor pullback takes :code:`d_coordinates` as an argument, which can be | ||
| used to identify the class for which the constructor pullback is defined. |
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.
Can we add a section on clad::zero?
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 think you meant clad::zero_init?
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.
Yes.
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 have mentioned why the constructor pullback custom derivatives does not have the initialization problem.
Currently, the docs does not cover object construction through custom derivatives. I would like to cover this topic as a follow-up PR.
|
|
||
| Constructor pullback custom derivatives are more similar to the ordinary pullback | ||
| functions. Constructor pullback functions do not have the same problem as of constructor | ||
| pushforward functions of initializing the primal object and the derivative object. After all, |
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.
Sadly, constructor_pullback doesn't have the same problem because the initialization must be done in the forward pass, and so this responsibility lies on constructor_reverse_forw. By the way, we need a section about that. We can often avoid using that by
A a(args);
-->
A a(args);
A da(a);
clad::zero_init(a);
But really, we can only avoid constructor_reverse_forw either for iterable containers, simple numerical structures (no pointer fields) like std::pair, or when the object is initialized with the default constructor. For complex types like std::vector, we rely on constructor_reverse_forw.
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 have mentioned why the pullback constructors does not have the same problem of initialization as of pushforward constructors.
Currently, the docs does not cover object construction through custom derivatives. I would like to cover this topic as a follow-up PR.
|
Thank you for the detailed reviews @vgvassilev and @PetroZarytskyi. I will implement the reviews soon. |
|
@arima-04, ping. |
ec8cbaf to
765fb95
Compare
765fb95 to
7e26619
Compare
Signed-off-by: Maki Arima <[email protected]>
7e26619 to
3b0133d
Compare
|
@vgvassilev @PetroZarytskyi Apologies for the delay. I have applied all the reviews suggestions. |
|
for english checking - could you first run the text through google docs and resolve the grammar suggestions that it has (it is likely at least as good as me) |
|
@arima-04, ping. |
I will get back to this PR tomorrow. I couldn't get a chance to run the documentation through google docs. |
No description provided.