Remove clad::array_ref from the reverse, hessian, jacobian and error estimation modes #758
Conversation
vgvassilev
force-pushed
the
remove-array-ref
branch
from
5dfcdf6
to
d339638
Compare
|
That may also fix #762. |
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
from
3da9341
to
20e2b32
Compare
There was a problem hiding this comment.
This looks really good. Very impressive work, especially the work involving making the correct call to gradient overload using template metaprogramming.
It's good to see this feature close to complete. Removing the dependency of clad::array_ref for representing derivatives will simplify lots of things.
This is only an initial review. I will need some more time for the complete review.
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
2 times, most recently
from
db91298
to
4cef797
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #758 +/- ##
==========================================
- Coverage 94.95% 94.81% -0.14%
==========================================
Files 49 49
Lines 7467 7490 +23
==========================================
+ Hits 7090 7102 +12
- Misses 377 388 +11
... and 1 file with indirect coverage changes
|
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
2 times, most recently
from
ee75866
to
edb09b0
Compare
|
@PetroZarytskyi, can you rebase this PR? |
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
6 times, most recently
from
f61d650
to
6242f82
Compare
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
2 times, most recently
from
21c688f
to
4915afb
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -42,6 +42,12 @@ void MultiplexExternalRMVSource::ActAfterParsingDiffArgs( | |||
| } | |||
| } | |||
|
|
|||
| void MultiplexExternalRMVSource::ActAfterProcessingArraySubscriptExpr( | |||
| const clang::Expr* revArrSub) { | |||
| for (auto source : m_Sources) | |||
There was a problem hiding this comment.
warning: 'auto source' can be declared as 'auto *source' [llvm-qualified-auto]
| for (auto source : m_Sources) | |
| for (auto *source : m_Sources) |
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
from
4915afb
to
3e46ac5
Compare
… invalid source location assertions
…ef adjoint parameters
PetroZarytskyi
force-pushed
the
remove-array-ref
branch
from
3e46ac5
to
d6dbaa4
Compare
There was a problem hiding this comment.
That's a massive step forward, thank you, @PetroZarytskyi. The patch looks mostly good to me. I'd wait for @parth-07 to review it and then we can move forward.
| }; | ||
| template <std::size_t N, typename... Args> | ||
| constexpr auto TakeNFirstArgs(list<Args...>) | ||
| -> decltype(TakeNFirstArgs<Args...>(MakeIndexSequence<N>{})); |
| @@ -0,0 +1,33 @@ | |||
| // RUN: %cladnumdiffclang %s -I%S/../../include -oInterfaceCompatibility.out 2>&1 | FileCheck %s | |||
There was a problem hiding this comment.
Looks great to me. It's great to see this finally landing. Thank you!
A small side note: The function traits have become really complicated now. You are perhaps one of the very few people who understands that code completely now. I remember the time when the function traits used were incredibly basic and were just used for handling minor things. The function traits have grown so much now.
@PetroZarytskyi, can you open an issue for this part of the comment? I believe it was expanded due to |
This PR removes
clad::array_reffrom the derivative code in all reverse modes. This is done to simplify the interface and make the code more efficient. This should also make it easier to add support forCand improve pointer support. All new pointer interfaces will remain compatible with oldarray_refinterfaces. In the error estimation mode,arrayRef.size()was replaced witharrayRef_sizeto track the biggest index ofarrayRefused in the gradient.Partially fixes #726, #274.