Differentiate array parameters in forward mode #1062
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1062 +/- ##
==========================================
- Coverage 94.40% 94.39% -0.02%
==========================================
Files 55 55
Lines 8430 8468 +38
==========================================
+ Hits 7958 7993 +35
- Misses 472 475 +3
... and 1 file with indirect coverage changes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// elements if the current array size is less than `size`. | ||
| CUDA_HOST_DEVICE void extend(std::size_t size) { | ||
| if (size > m_size) { | ||
| T* extendedArr = new T[size]; |
There was a problem hiding this comment.
warning: initializing non-owner 'T *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
T* extendedArr = new T[size];
^
PetroZarytskyi
force-pushed
the
issue-1035
branch
from
74affff to
00a9790
Compare
vgvassilev
reviewed
Aug 29, 2024
| // but we need to be able to differentiate it to prevent segfaults in | ||
| // hessians. Once we add support for methods that change their objects, this | ||
| // section should be removed. | ||
| if (FDName == "extend") |
There was a problem hiding this comment.
Can we not provide a pullback for this interface?
There was a problem hiding this comment.
I thought this was not possible but I forgot we now support custom _forw functions. I fixed this. Thank you for pointing this out.
PetroZarytskyi
force-pushed
the
issue-1035
branch
from
00a9790 to
9b56257
Compare
| @@ -446,6 +461,20 @@ operator/(const array<T>& arr1, const array<U>& arr2) { | |||
| arr2); | |||
| } | |||
|
|
|||
| namespace custom_derivatives { | |||
| namespace class_functions { | |||
There was a problem hiding this comment.
warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
Suggested change
| namespace class_functions { | |
| namespace custom_derivatives::class_functions { |
include/clad/Differentiator/Array.h:474:
- } // namespace class_functions
- } // namespace custom_derivatives
+ } // namespace custom_derivatives
vgvassilev
force-pushed
the
issue-1035
branch
from
956c862 to
cae7c17
Compare
vgvassilev
force-pushed
the
issue-1035
branch
from
cae7c17 to
66717fe
Compare
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.
Currently, we don't create an adjoint of an array parameter in the forward mode because we don't know the size of the array. This means considering it non-differentiable, which is not always the case. This PR does two things:
double[5]), use the same type for the adjoint.There are downsides to the approach in 2. : extending the array involves reallocating memory, which costs performance and will cause a segfault if the user stored a pointer to some of the array's elements. However, alternatives are banning array parameters or considering them non-differentiable as before, which will fail way more frequently and produce warnings at max.
Also, please note the function
extendthat I introduced is not the same asresizesince the latter can shrink the array if its argument is less than the size of the array.extendonly makes the array bigger.Fixes #1035.