-
-
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 for the meta functions #1332
Conversation
library methods or coalesce them to be more consistent
…gs/RELEASE_500/final)
…gs/RELEASE_500/final)
…gs/RELEASE_500/final)
…stable/2017-11-14)
This reverts commit a39fa30.
…stable/2017-11-14)
This seems like it will go green. If you dont have anyone else in mind for review feel free to assign me. I have enough experience with the traits in Stan Math that I feel comfortable with this. |
Awesome ty! Yeah I'll assign you if you don't mind |
T, std::enable_if_t<std::is_const<T>::value || std::is_pointer<T>::value>> { | ||
using type = typename scalar_type_base< | ||
std::remove_pointer_t<std::remove_cv_t<T>>>::type; | ||
}; |
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.
@bob-carpenter @syclik this is probs the only serious change to point out. I'm assuming the intent of scalar_type
is to get the underlying value so this also cycles through pointers. Does this seem like it's right?
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98) |
…gs/RELEASE_500/final)
…stable/2017-11-14)
…stable/2017-11-14)
Edit: Nvm we are good!
|
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.97) |
Glad this was solved, I will look into it to see why this happened. |
@SteveBronder will have a look this evening or tomorrow morning. |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.96) |
Awesome! I glanced at it and it looks rad. (@rok-cesnovar, stay on as the reviewer please. Thank you!) |
Sorry Steve, I havent gotten to this yet. Will do tmrw. |
No worries! |
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 is fantastic stuff Steve! Some minor stuff on the tests and one or two comments.
…includes from from scal and arr tests. Fixed docs for is_var.hpp
Summary
This refactors the metaprogramming structs and aliases to be a bit more consistent in their definitions and use more of the C++14 standard library methods. A few of the highlights are
Instead of using partial specializations that required explicit specializations for const and & types, we use SFINAE and
enable_if_t<std::decay_t<T>>
to turn on and off the specializations for things likeis_vector
Edit:
scalar_type
removes const but leaves refs and pointers which is what the original implementation didscalar_seq_view
now uses SFINAE to make sure types use the correct specialization.Deletes ad_promotable in fwd and rev. Instead we use the
std::is_convertible
check that the types are implicitly convertible.is_fvar depended on knowing fvar, but the
.hpp
of fvar depends on the meta folder which defined fvar! So I madefvar.hpp
not depend on the meta programming.new structs
is_std_vector
andis_eigen
to check if something is a standard vector or derived from EigenBaseis_vector
now checks if the eigen vector has rows or columns at compile time equal to 1.Moved a number of metaprogramming from arr and mat into scale when it could be instantiated there with specializations for
vector
andeigen
left inarr
andmat
Cleaned up some of the docs
The new struct
bool_constant
, which is juststd::integral_constant<bool, bool B>
, is inherited by most of the structs that are used to check for types at compile time.Some of the tests checked types for
std::vector<const double>
orstd::vector<double&>
which is illegal under C++11 (types in std::vectors have to be movable and erasable so these were changed to look for pointers.structs containing a type now use
using
instead oftypedef
13. constexpr helper variable templates so we can call(THX WINDOWS)is_vector_v<T>
instead ofis_vector<T>::value
Tests
The only thing I still need to do is update the tests for
is_std_vector
andis_eigen
Side Effects
Should be none
Checklist
Math issue Update internals to use more modern c++ #1308
Copyright holder: Steve Bronder
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make doxygen
)make cpplint
)(YAS) the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested