make var_views assignable #2065
Conversation
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
There was a problem hiding this comment.
Looks like I went a little coo-coo trying to be const correct here when it was not needed lol. Just a note to add a construct so
var_value<Eigen::MatrixXd> A_block = A_v.block(0, 0, 3, 3);works and then this looks good! I think we will need that for the compiler unless we can get away with using auto in the generated code
it would also be good to add more tests, though I can add a bunch of stress tests in #2064
| template <typename S, require_convertible_t<S&, value_type>* = nullptr, | ||
| require_any_not_plain_type_t<T, S>* = nullptr> | ||
| var_value<T> operator=(const var_value<S>& other) { |
| TEST_F(AgradRev, var_matrix_view_assignment) { | ||
| Eigen::MatrixXd A(4, 4); | ||
| A << 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15; | ||
| stan::math::var_value<Eigen::MatrixXd> A_v(A); | ||
| A_v.block(0, 0, 3, 3) = A_v.block(1, 1, 3, 3); | ||
| stan::math::sum(stan::math::from_var_value(A_v)).grad(); | ||
| Eigen::MatrixXd deriv(4, 4); | ||
| deriv << 0, 0, 0, 1, 0, 1, 1, 2, 0, 1, 1, 2, 1, 2, 2, 2; | ||
| EXPECT_MATRIX_FLOAT_EQ(A_v.adj(), deriv); | ||
| } |
There was a problem hiding this comment.
Adding stan::math::var_value<Eigen::MatrixXd> A_block = A_v.block(0, 0, 3, 3); fails with
test/unit/math/rev/core/var_test.cpp: In member function ‘virtual void AgradRev_var_matrix_view_assignment_Test::TestBody()’:
test/unit/math/rev/core/var_test.cpp:228:61: error: conversion from ‘var_value<Eigen::Block<Eigen::Map<Eigen::Matrix<double, -1, -1>, 8, Eigen::Stride<0, 0> >, -1, -1, false>>’ to non-scalar type ‘var_value<Eigen::Matrix<double, -1, -1>>’ requested
228 | stan::math::var_value<Eigen::MatrixXd> A_block = A_v.block(0, 0, 3, 3);
I think we just need a little constructor to do that
/**
* Construct a `var_value` with an inner plain matrix type from a `var_value`
* holding an expression of another matrix.
* @tparam S A type representing an eigen expression
* @param other An eigen expression.
*/
template <typename S, require_convertible_t<S&, value_type>* = nullptr,
require_not_plain_type_t<S>* = nullptr,
require_not_same_t<T, S>* = nullptr>
var_value(const var_value<S>& other) : vi_(new vari_type(other.vi_->val_)) {
reverse_pass_callback(
[this_vi = this->vi_, other_vi = other.vi_]() mutable {
this_vi->adj_ += other_vi->adj_;
});
}There was a problem hiding this comment.
Fixed and added a test.
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Yeah I tend to be a bit lazy with tests. I think this is enough to show it works. If you want to add more, feel free. Or if you want me to add something specific, feel free to ask. I also intend to add something similar for scalars. However, I wan to avoid merge conflicts, so I will wait until #2064 is merged. Also it contains some changes I'd need to do anyway. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@SteveBronder This is waiting for next review. |
There was a problem hiding this comment.
So I think this looks good. Splitting out var_value can either be done in this PR or #2064
Summary
Makes it possible to assign to blocks, and other views into
var_value(does not include assignments of single elements).Tests
Tested assignment of one block to another that contains aliasing.
Side Effects
With this there is no reason anymore to call
var_valuea static matrix :)Release notes
Blocks, and other views into
var_valuecan now be assigned to.Checklist
Math issue How to add static matrix? #1805
Copyright holder: Tadej Ciglarič
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested