Skip to content
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

[WIP] Use auto and more general template types #1309

Closed
wants to merge 60 commits into from

Conversation

SteveBronder
Copy link
Collaborator

@SteveBronder SteveBronder commented Aug 14, 2019

Summary

As described in this discourse thread I think we can get away with using auto in the return types for simple functions like add, subtract, and multiply. But to do that accept general Eigen types I had to do a few things

  1. For simple functions like add, subtract, multiply, we use a signature that looks like
template <typename T1, typename T2, int R, int C>
inline Eigen::Matrix<return_type_t<T1, T2>, R, C> add(
    const Eigen::Matrix<T1, R, C>& m1, const Eigen::Matrix<T2, R, C>& m2) {

We now use

template <typename Mat1, typename Mat2, typename = enable_if_all_eigen<Mat1, Mat2>>
inline auto add(const Mat1& m1, const Mat2& m2) {

This is nice because enable_if_all_eigen checks that the template type inherits from EigenBase, which means that Mat1 and Mat2 can be types like Eigen::Product<...> or Eigen::Diagonal<...>. In order to differ to the correct function a large number of enable_if_* methods have been made available.

  1. In order to make sure I'm pulling out the types and sub types correctly I had to update the metaprogramming structs so that the eigen based meta programming methods work with more than just eigen matrices. In general I tried to go through them and use things like std::decay_t so we don't need specializations for const etc.

  2. By cleaning up the metaprogramming we can also have the apply_scalar_unary also support any type of Eigen matrix. This should remove a lot of copies and let us use Sparse matrices in the future.

  3. Most of the functions in fwd are really specializations for Eigen matrices with fvar types. With the use of this refactor we can remove a lot of those methods in fwd and default to the prim implementations. There are only one or two methods which lose precision (to the level of 1e-15) and I'll doc/note those in this PR so we can talk about whether it's worth having another method and more precision or just using the default methods in prim.

(on a side note, maybe not for this PR but it may be nice to change the name of prim to generic since that's the real intent of that folder)

Tests

I think the original tests should be fine? I would like to also add tests for arithmetic sparse matrices since that's the intent of this PR

Side Effects

Not sure if it's a side effect, but these methods will now work for all classes that inherit from EigenBase.

Checklist

  • Math issue Update internals to use more modern c++ #1308

  • Copyright holder: Steve Bronder

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@SteveBronder
Copy link
Collaborator Author

@syclik I think I need your opinion on this. One important point of this PR is that we should only do this for functions where we don't create a true temporary with .eval(). So add, subtract, multiply are all p safe but other functions we'll need to look over

EXPECT_TRUE(is_vector<std::vector<const double> >::value);
EXPECT_TRUE(is_vector<std::vector<const int> >::value);
EXPECT_TRUE(is_vector<const std::vector<double> >::value);
EXPECT_TRUE(is_vector<const std::vector<int> >::value);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syclik fyi I changed these because it looks like having const types in C++11 is illegal?

https://stackoverflow.com/questions/6954906/does-c11-allow-vectorconst-t

@SteveBronder
Copy link
Collaborator Author

Also the scalar unary tests for rev and fwd were .hpp instead of .cpp so I changed them to .cpp so they would go off

@syclik
Copy link
Member

syclik commented Aug 14, 2019

@SteveBronder, what opinion did you need? I didn't quite understand. I think this is good, if we can make sure the results are correct. That's going to be hard to validate, but we really should.

@@ -85,7 +85,7 @@ TEST(AgradMixMatrixLogDeterminant, ffv_1stDeriv) {
EXPECT_FLOAT_EQ(-1.5, h[0]);
EXPECT_FLOAT_EQ(1, h[1]);
EXPECT_FLOAT_EQ(.5, h[2]);
EXPECT_FLOAT_EQ(0.0, h[3]);
EXPECT_NEAR(0.0, h[3], 1e-15);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this level of precision okay or should we keep the fwd implementation?

EXPECT_FLOAT_EQ(4.0, h[0]);
EXPECT_FLOAT_EQ(-2.0, h[1]);
EXPECT_FLOAT_EQ(-1.0, h[2]);
EXPECT_FLOAT_EQ(0.0, h[3]);
EXPECT_FLOAT_EQ(0.0, h[4]);
EXPECT_FLOAT_EQ(0.0, h[5]);
}
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to go through and double check, but a lot of these tests failed at run time where they now fail at compile time (since a lot of these methods now check in the their template that the Eigen matrices are vectors

@@ -108,7 +86,7 @@ struct apply_scalar_unary<F, int> {
* @param x Argument scalar.
* @return Result of applying F to the scalar.
*/
static inline return_t apply(int x) { return F::fun(static_cast<double>(x)); }
static inline return_t apply(T x) { return F::fun(static_cast<double>(x)); }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I totally get the intuition here for the static_cast<double>

@serban-nicusor-toptal serban-nicusor-toptal added this to the 3.0.0 milestone Oct 18, 2019
@t4c1 t4c1 mentioned this pull request Nov 29, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants