Implement new autodiff type based on tape representation #89
Conversation
expression - class for tape based representation which supports update.
…nd differential properties
…variable from numerical value
There was a problem hiding this comment.
Hi @supersega - very cool! I've left some comments and suggestions. I think what we need now is more work on:
- tests (from simple to complex ones, considering also higher-order derivatives)
- tutorials
- a bit of documentation on how the type works.
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| // SOFTWARE. | ||
|
|
||
| #include <autodiff/mixed/algorithms/first_order.hpp> |
There was a problem hiding this comment.
For file names, so far we have adopted a no-underline, no-caps convention, meaning, we would use firstorder.hpp.
There was a problem hiding this comment.
I believe that _ will help to reed file name, but if we prefer such naming as you mentioned I will change.
| { | ||
| namespace detail | ||
| { | ||
| /// @brief Meta function to generate tuple which contains derivatives array. |
There was a problem hiding this comment.
What about setting JAVADOC_AUTOBRIEF to YES, so @brief is not needed?
There was a problem hiding this comment.
We should just arrange about documentation style, and I will do ;)
| namespace autodiff::taperep | ||
| { | ||
| /// @brief Class which represent tape based storage for expression tree. | ||
| /// @tparam N order of tape. |
There was a problem hiding this comment.
Say, if N == 2, does this means one can compute 2nd order derivatives, say, Hessian? How do you deal with all possible combinations of cross derivatives, and the symmetry among them (e.g., ∂²u/∂x∂y == ∂²u/∂y∂x)? This looks quite interesting.
There was a problem hiding this comment.
Yes, we can. I can send publication where algorithm is described.
| /// @brief Construct variable from arithmetic types. We need it because Eigen requires it. | ||
| /// @details In Eigen code they have check in if statement, where compiler | ||
| /// looking for Scalar(0). Our constructor is empty. | ||
| template<typename U, std::enable_if_t<std::is_arithmetic_v<U>>...> |
There was a problem hiding this comment.
This will most likely need to change to permit custom floating-point types to be used.
There was a problem hiding this comment.
Yes, we must support custom floating point numbers, but we can do it in someone of future PRs.
|
|
||
| /// @brief Get value for var. | ||
| /// @return Numerical value of variable. | ||
| auto value() const -> const double& |
There was a problem hiding this comment.
double also will need to be templated (either in this PR or in a future one).
There was a problem hiding this comment.
We can do it in next PR to avoid monsters =)
| /// @brief Binary sum expression. | ||
| /// @tparam N Maximal derivative order of expression. | ||
| template<std::size_t N> | ||
| struct sum_expression final : binary_expression<N> |
There was a problem hiding this comment.
To me, sum means a summation of numbers (e.g., sum operator, sum symbol in LaTeX, etc.). I think this should be addition_expression.
| /// @brief Binary minus expression. | ||
| /// @tparam N Maximal derivative order of expression. | ||
| template<std::size_t N> | ||
| struct minus_expression final : binary_expression<N> |
There was a problem hiding this comment.
Here, we should use subtraction_expression if the intent is x - y. I think minus_expression makes sense for the unary operation -x.
| /// @brief Update value of divide expression. | ||
| /// @details Only first order derivatives are need recalculation, | ||
| /// since other are always zero. | ||
| virtual void update() |
There was a problem hiding this comment.
Wondering here if these update methods can deal with branches (e.g., if conditions)?
There was a problem hiding this comment.
Also can be introduced in new PR
|
|
||
| namespace autodiff::taperep { | ||
|
|
||
| constexpr auto first_order = 1; ///< Constant to get first order partial derivatives. |
There was a problem hiding this comment.
Do we really need these two constants? Early on during this review, I found partial_derivatives<first_order>() and my impression was that first_order was a type. partial_derivatives<1>() would have been clearer. But this is a bit subjective. Maybe we could name order1st, order2nd, order3rd, order4th, etc.
There was a problem hiding this comment.
I like first_order. I can read it like phrase from book.
| @@ -1,2 +1,3 @@ | |||
| add_subdirectory(forward) | |||
| add_subdirectory(reverse) | |||
| add_subdirectory(mixed) | |||
There was a problem hiding this comment.
What is the meaning of mixed? What would you propose as alternative names so we can decide between them?
There was a problem hiding this comment.
We can travel in both directions.
|
Hi @allanleal, I want to replace only most serious moments like naming style etc. What I suggest to move into future PRs?
What I suggest to fix in this PR?
|
|
@allanleal There has not been any work on this mr for quite some time. Is this still a wanted feature? If so how can I help? Should I create a copy of this mr and improve on it? Is there a way to split it into smaller chunks? Maybe a milestone with multiple smaller steps in-between which could be iteratively merged? |
|
Could you comment on the potential advantages of this new type? I am in particular interested in obtaining all the second and third partial derivatives of a function of two variables in one shot, and wonder whether this would be superior to multiple calls with forward differentiation. |
|
Ian, let's talk about this (please email me). I think I have a better solution for you (which we could test first before releasing). I assume your use case is for thermodynamics; and the two variables are perhaps temperature and density?! |
|
Correct - email headed your way |
Description
This PR introduce new automatic differentiable variable.
Type of change