-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Generalize */fun starting with cr-d #1754
Conversation
…gs/RELEASE_500/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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.
Mostly optional comments and a few doc changes.
@andrjohns would you mind taking a quick look at the rev stuff? I think you know rev
better than I do. Nothing here seems weird at all but better to have a double check
stan/math/fwd/fun/determinant.hpp
Outdated
template <typename T, require_eigen_vt<is_fvar, T>* = nullptr> | ||
inline value_type_t<T> determinant(const T& m) { |
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.
EigMat
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.
done
stan/math/fwd/fun/determinant.hpp
Outdated
check_square("determinant", "m", m); | ||
|
||
const T vals = m.val().determinant(); | ||
return fvar<T>(vals, vals * (m.val().inverse() * m.d()).trace()); | ||
const typename value_type_t<T>::Scalar vals = m.val().determinant(); |
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.
?
const typename value_type_t<T>::Scalar vals = m.val().determinant(); | |
const scalar_type_t<T> vals = m.val().determinant(); |
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.
Nope. value_type_t<T>
is fvar<X>
and I need X
here.
stan/math/fwd/fun/tcrossprod.hpp
Outdated
inline Eigen::Matrix<value_type_t<T>, T::RowsAtCompileTime, | ||
T::RowsAtCompileTime> |
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.
Do we want to use auto
here and .matrix()
on the return of multiply?
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.
Also EigMat
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.
fyi everywhere T is an Eigen Matrix would be good to have a nicer name like EigMat
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.
Done. I know about template names, but they are not very high on my priority list, so I tend to forget to change them. You can either keep reminding me about every single instance I miss or we can leave that for a separate series of PRs.
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.
One of the return statements is invoking default constructor, so compiler can not deduce auto
.
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.
Except at moment the function will take any eigen type as an input but can only return Eigen::Matrix
types (which could cause some compile errors down the line). You could restrict the inputs to only take matrix types, or since the result will need to be evaluated anyway, this could be written to be more flexible:
template <typename T, require_eigen_vt<is_fvar, T>* = nullptr>
inline auto tcrossprod(const T& m) {
if (m.rows() == 0) {
return plain_type_t<decltype(multiply(m, m.transpose()))>{};
}
return multiply(m, m.transpose()).eval();
}
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.
Many functions still assume input is derived from Eigen::MatrixBase
and will not work with Eigen::ArrayBase
-derived types. Or are you thinking about some other type this would fail with?
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.
No it was just ArrayBase
that I was thinking of, so if that's a non-issue then feel free to ignore
* @param x1 First scalar. | ||
* @param x2 Second scalar. | ||
* @return Squared distance between scalars | ||
* @throw std::domain_error Any scalar is not finite. |
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.
Would you mind documenting T1 and T2 as well.
[Optionally] Could also make the names something nicer like Scalar1
and Scalar2
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.
done
template <typename T1, typename T2, | ||
require_eigen_vector_vt<is_var, T1>* = nullptr, | ||
require_eigen_vector_vt<std::is_arithmetic, T2>* = nullptr> | ||
inline var squared_distance(const T1& v1, const T2& v2) { |
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.
[Optional] For something like this I think EigenVar
and EigenArith
would be useful names
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.
done
Eigen::Matrix<double, Eigen::Dynamic, Eigen::Dynamic> m1(1, 1); | ||
m1 << 2.0; | ||
EXPECT_NEAR(4.0, dot_self(m1), 1E-12); |
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.
Why gone?
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.
Oh because it only takes in vectors now? I wonder if this will break user code tho?
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.
Right. I should probably let this accept matrices too, even if matrix version is not exposed to stan language.
The rev stuff all looks pretty great. Should we also replace the:
with
Or save that for a separate issue? |
I would prefere to leave this one out of this one. I am already doing quite a few things at once here. |
# Conflicts: # stan/math/fwd/fun/columns_dot_self.hpp # stan/math/fwd/fun/rows_dot_self.hpp # stan/math/fwd/fun/squared_distance.hpp # stan/math/prim/fun/distance.hpp # stan/math/prim/fun/squared_distance.hpp
…gs/RELEASE_500/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@t4c1 There's a problem with using
Which returns:
The only fix I've found is to change the |
Noticed the error after I hit send, the test has a typo ( |
Alright, I wasn't entirely crazy, there is an issue with |
With the example:
I'm not exactly sure what the reference structure for expressions is, but I'm guessing that Also, why is |
That makes sense, going to make generalising these functions safely an interesting proposition.
More info on that here |
One solution I've found is to detect whether an expression has been passed to the function, and if so, evaluate the expression prior to calling the function: https://godbolt.org/z/jqoFZi Whether this is the best (or most efficient) solution is another question |
First thanks for finding this. I don't like your suggestion. It gives up all the benefits of using I guess we could instead place that EDIT: Also it might be better to put this discussion on |
Yes, passing Eigen structures around efficiently deserves its own independent discussion. I don't see a good option. The two under consideration are.
What we really need is a kind of inlining through expression templates. |
@SteveBronder this is waiting for another review. The issue with local variables only blocks returning expressions. |
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.
Looks good! One actual fix but most of the comments are optional. Would be nice to have T* -> EigMat* or EigT* so we can read the signature and see what types this operates on
stan/math/fwd/fun/determinant.hpp
Outdated
const T vals = m.val().determinant(); | ||
return fvar<T>(vals, vals * (m.val().inverse() * m.d()).trace()); | ||
const typename value_type_t<EigMat>::Scalar vals = m.val().determinant(); | ||
return value_type_t<EigMat>(vals, vals * (m.val().inverse() * m.d()).trace()); |
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.
[optional]
Since you declare the return type at the top you could do
return value_type_t<EigMat>(vals, vals * (m.val().inverse() * m.d()).trace()); | |
{vals, vals * (m.val().inverse() * m.d()).trace()}; |
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.
fixed
stan/math/prim/fun/crossprod.hpp
Outdated
template <typename T, require_eigen_t<T>* = nullptr> | ||
inline auto crossprod(const T& M) { | ||
return tcrossprod(M.transpose()); |
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.
[optional]
T -> EigMat
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.
fixed
inline Eigen::Matrix<value_type_t<T>, T::RowsAtCompileTime, | ||
T::ColsAtCompileTime> |
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.
[optional]
I think auto is fine here since the type is declared in the function
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.
fixed
stan/math/prim/fun/diag_matrix.hpp
Outdated
template <typename T, require_eigen_vector_t<T>* = nullptr> | ||
inline Eigen::Matrix<value_type_t<T>, Eigen::Dynamic, Eigen::Dynamic> | ||
diag_matrix(const T& v) { |
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.
[optional] T -> EigMat
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.
fixed
template < | ||
typename EigVecVar1, typename EigVecVar2, | ||
require_all_eigen_vector_vt<is_var, EigVecVar1, EigVecVar2>* = nullptr> | ||
inline var squared_distance(const EigVecVar1& v1, const EigVecVar2& v2) { | ||
check_matching_sizes("squared_distance", "v1", v1, "v2", v2); | ||
return var(new internal::squared_distance_vv_vari(v1, v2)); |
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.
[optional]
return type can let this be
return var(new internal::squared_distance_vv_vari(v1, v2)); | |
return {new internal::squared_distance_vv_vari(v1, v2)}; |
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.
fixed
…stable/2017-11-14)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@SteveBronder This is ready for review. |
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.
Two optional comments but looks good to me!
stan/math/prim/fun/determinant.hpp
Outdated
inline value_type_t<T> determinant(const T& m) { | ||
check_square("determinant", "m", m); | ||
if (m.size() == 0) { | ||
return 1; |
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.
Two things here that I think are both optional
- Looking at Eigen's determinant code it looks like they look for a 0 size as well so idt we need the m.size() check here
https://gitlab.com/libeigen/eigen/-/blob/master/Eigen/src/LU/Determinant.h#L32
static inline typename traits<Derived>::Scalar run(const Derived& m)
{
if(Derived::ColsAtCompileTime==Dynamic && m.rows()==0)
return typename traits<Derived>::Scalar(1);
return m.partialPivLu().determinant();
}
- If we keep the if then I think doing
auto
at the top andstatic_cast<value_type_t<T>>(1)
would enable rvo here.
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.
Caring about rvo when returning a scalar seems a bit pointless. I will check if removing if breaks anything.
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.
Yeah idt it's a big deal either though I think it's good for style imo. That's why it's optional
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.
lgtm!
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
Generalizes functions with names starting with cr-d.
Tests
Some checks that an input is a vector are now done at compile time instead of runtime. Tests fpr these checks are removed.
Side Effects
None.
Checklist
Math issue Generalize matrix function signatures #1470
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