-
-
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
Refactor Eigen Type traits #1667
Conversation
…nherits from Eigen::DenseBase with more than 1 row and column
…stable/2017-11-14)
Conceptually, wouldn’t it be better if we called this |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
What if we just switched it to I think with the direction things are going we wouldn't want an explicit specialization for @t4c1 ? |
I agree with @syclik. I would prefere to keep the name
What exactley do you mean by switch to it? |
What are you trying to match. What will is_eigen_and_not_vector match besides Matrix<T, -1, -1>?
… On Feb 3, 2020, at 3:03 AM, Tadej Ciglarič ***@***.***> wrote:
I agree with @syclik. I would prefere to keep the name is_eigen_matrix free in case we ever need it to distinguish between Eigen Matrix and Array. I would propose to name the thing you are trying to do here is_eigen_and_not_vector or something similar.
What if we just switched it to Eigen::MatrixBase?
What exactley do you mean by switch to it?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
It will also match any expression that evaluates to |
How about is_eigen_matrix_assignable instead or is_assignable_eigen_matrix to keep our naming in line with std::is_assignable?
Or given that we don't have other types of matrices, we could just drop the "eigen" here.
… On Feb 3, 2020, at 3:03 AM, Tadej Ciglarič ***@***.***> wrote:
I agree with @syclik. I would prefere to keep the name is_eigen_matrix free in case we ever need it to distinguish between Eigen Matrix and Array. I would propose to name the thing you are trying to do here is_eigen_and_not_vector or something similar.
What if we just switched it to Eigen::MatrixBase?
What exactley do you mean by switch to it?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
idk if I like having the concept be named by what it's not. I have to think about that. How about " So the above fits for dynamic Eigen matrices and fixed size matrices. Does that conceptually/practically seem clear enough? Though it leaves a
Jus switching the above definition to say Eigen::MatrixBase
Are there other places we use
|
@SteveBronder : I think our suggestions crossed in the ether. I prefer |
I think that is the only way not to be ambiguous. The problem is that row and column vectors are special cases of matrices and I don't think we have a word for a matrix that is not a vector. Calling it either |
Despite the superficial similarity, there are three distinct overloads of
We can use "vector" for either row or column vectors, though in informal math writing, when we say "vector" we usually mean column vectors. |
If we use suggested terminology the term "matrix" becomes ambiguous. It could mean any matrix or just |
There's only one matrix, Matrix<T, -1, -1>. The other two classes are vectors.
… On Feb 3, 2020, at 3:02 PM, Tadej Ciglarič ***@***.***> wrote:
If we use suggested terminology the term "matrix" becomes ambiguous. It could mean any matrix or just Matrix<T,-1,-1>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I think there's two points to call here
how would you want to correct the below definition? Or is it just the name?
ftr I'm thinking along Bob's line in the below where computationally we treat vectors different conceptually in the codebase
|
Exactly. It's like the relation between integers and real numbers, or between reals and complex.
This is tricky. Eigen itself only recommends doing this for very small matrices (single digit sizes) because everything would otherwise be allocated on the stack, which is of very limited size, especially with multithreading (on the order of 20KB, so anything beyond about 50 x 50 is going to blow out stack memory).
This explicitly doesn't consider
|
…ase and has neither row nor columns equal to 1
…stable/2017-11-14)
…th into feature/require-eigen-matrix
Mulling this over I like this definition
Though I changed it slightly to be just
imo I think this makes sense because this is the base class for eigen matrices. See below for Eigen's reference on that https://eigen.tuxfamily.org/dox/TopicFunctionTakingEigenTypes.html |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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.
More doc and testing please. Also, I'd like to hear from @syclik about conventions for splitting functions across files.
@@ -102,6 +102,13 @@ template <template <class...> class TypeCheck, class... Check> | |||
struct is_eigen_value_check | |||
: container_value_type_check_base<is_eigen, TypeCheck, Check...> {}; | |||
|
|||
/** | |||
* Meta type trait to check if type inherits from EigenBase. |
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.
These should have their template parameters documented as well as what kind of value they're going to produce. Same for all the rest of these traits classes.
@@ -799,6 +834,30 @@ template <template <class...> class TypeCheck, class... Check> | |||
using require_all_not_eigen_vt |
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.
This really needs doc because I have no idea what the v
is supposed to signal in vt
.
@@ -935,6 +994,30 @@ template <template <class...> class TypeCheck, class... Check> | |||
using require_all_not_eigen_st | |||
= require_all_not_t<is_eigen_scalar_check<TypeCheck, Check>...>; | |||
|
|||
template <template <class...> class TypeCheck, class... Check> |
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.
What does the s
mean? I see is_eigen_matrix_scalar_check
but I don't know what that means, either as it seems to be mixing types (matrix and scalar).
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.
The st
stands for scalar_type
it's doc'd on the website
http://mc-stan.org/math/d1/db9/group__require__meta.html
This PR is already pretty big so I'm going to open up another PR to move these
@@ -39,17 +39,25 @@ TEST(MathMetaPrim, expression) { | |||
EXPECT_FALSE((is_eigen_matrix<Eigen::EigenBase<Eigen::MatrixXd>>::value)); |
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.
Every metaprogram needs tests.
@SteveBronder : Is something holding this up? I'd rather not leave PRs dangling for weeks with no activity. |
Apologies I've just been running around with job stuff I'll put fix this up today |
T, std::enable_if_t<internal::is_eigen_base<std::decay_t<T>>::value>> | ||
struct is_eigen<T, std::enable_if_t<std::is_base_of< | ||
Eigen::EigenBase<typename std::decay_t<T>::PlainObject>, | ||
typename std::decay_t<T>::PlainObject>::value>> |
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.
Just a general comment, but it might make these templates more readable if the typename std::decay_t<T>::PlainObject
is moved up to the template definition:
template <typename T, typename PlainType = typename std::decay_t<T>::PlainObject>
struct is_eigen<T, std::enable_if_t<std::is_base_of<
Eigen::EigenBase<PlainType>,
PlainType>::value>>
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.
Agree!!
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.
Actually idt this would work since it would set the default of the template value but I have to grab PlainObject
in one specialization and MatrixType
in another
…xp1~20180509124008.99 (branches/release_50)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
This is ready for review |
template <typename T> | ||
using is_eigen_matrix_or_array | ||
= math::disjunction<is_eigen_matrix<std::decay_t<T>>, | ||
is_eigen_array<std::decay_t<T>>>; |
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.
With the change to is_eigen_matrix
this does not make sense anymore. It should be renamed to is_eigen_matrix_or_vector_or_array
ant the test should be changed appropriately.
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.
It seems like the only place we are using is_eigen_matrix_or_array
was in matrix_cl to check whether we should call .eval() or not. is_eigen_matrix_or_vector_or_array
seems like kind of a mouthful. Should we just do is_eigen_dense
in matrix_cl
and get rid of this type trait?
Code that uses `is_eigen_matrix_or_array:
https://github.com/stan-dev/math/blob/develop/stan/math/opencl/matrix_cl.hpp#L312
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.
is_eigen_dense
is not the same thing! But I agree we can find a better name. This is actually checking whether something is a plain type so maybe is_eigen_plain_type
?
I'm a little confused as to who's reviewing this now. Should I do the final review now? |
@bob-carpenter go ahead with the review. I just had one comment. |
I don't understand what any of those metaprograms do, so I still want doc. For example, I don't know what what the And I'm afraid I can't even parse these headers:
I think you may need someone with more skill in templates to review this. I'm totally slammed right now and this whole file's full of things I don't understand. |
…gs/RELEASE_500/final)
…th into feature/require-eigen-matrix
@t4c1 if this is too large to be nice to review I think I could break it up into at least 2 PRs with
Or (2) could be broken up into 2 more PRs as well |
I can take a look at this if @t4c1 doesn't have time. Let me know. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@bob-carpenter I you want to review, go ahead. Otherwise I can do it. |
I'm very fine with splitting this into 2 or 3 PRs |
If you could review, @tadej, that'd be great as I'm swamped as usual. |
Summary
Cleans up and expands the type traits for selecting certain Eigen types. This uses Eigen's inheritance structure to determine for each type trait whether the type we are checking inherits a particular Eigen base class. In order, the hierarchy for types is essentially
EigenBase
DenseBase
MatrixBase
Matrix
ArrayBase
Array
MapBase
LDLT
LLT
SparseMatrixBase
SparseCompressedMatrixBase
SparseMatrix
This PR includes functions to check types that inherit from
EigenBase
,DenseBase
,MatrixBase
,ArrayBase
,MapBase
, andSparseMatrixBase
.This PR also changes the definition of
is_eigen_matrix
to be anything that inherits fromEigen::MatrixBase
with more than one row and column. There were places where we want to specialize for eigen vectors while having other specialization for general eigen matrices. This meant we had to do something likeWhich is annoying. With this PR we can just write
Tests
Tests are done for generic eigen expressions inheriting from each type above and for all stan scalar types. They can be run with
Side Effects
Yes, the definition of is_eigen_matrix will now accept more generic eigen expressions.
Checklist
Math issue Update internals to use more modern c++ #1308
Copyright holder: Steve Bronder
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