Feature/issue 1452 new to matrix signatures #508

randommm · 2017-03-16T18:20:32Z

Submisison Checklist

Run unit tests: ./runTests.py test/unit/math/prim/mat/fun/to_matrix_test.cpp (hope it suffices)
Run cpplint: make cpplint
Declare copyright holder and open-source license: see below

Summary:

Fix issue #507

Implemented a bunch of new to_matrix functions

to_matrix(matrix, int, int)
to_matrix(vector, int, int)
to_matrix(row_vector, int, int)
to_matrix(int[], int, int)
to_matrix(matrix, int, int, int)
to_matrix(vector, int, int, int)
to_matrix(row_vector, int, int, int)
to_matrix(real[], int, int, int)
to_matrix(int[], int, int, int)

Intended Effect:

Make users life easier.

How to Verify:

Unit tests included.

Side Effects:

Almost none other than users (and me) starting to get confused with this bunch of function signatures.

Documentation:

Included.

Reviewer Suggestions:

Anyone

Copyright and Licensing

Marco Inacio

By submitting this pull request, the copyright holder is agreeing to 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/)

…row_vector, m, n)

…nt) and to_matrix(row_vector, int, int, int)

… int)

bob-carpenter · 2017-03-18T22:32:07Z

That was just a timeout on the last failure and I kicked off the test that failed again.

bob-carpenter

See comments inline. Most of these are just formatting. And thanks for the pull request!

bob-carpenter · 2017-03-19T23:32:57Z

stan/math/prim/arr/fun/promote_elements.hpp

@@ -3,6 +3,7 @@

 #include <stan/math/prim/scal/fun/promote_elements.hpp>
 #include <vector>
+#include <cstddef>


Why is cstddef being added? It looks like it would have passed header tests before.

Oddly, it was not passing on my local compiler either with g or clang (clang version 3.8.1-12ubuntu1 (tags/RELEASE_381/final) and g++ (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005 with lastest "stable" Ubuntu) complaining about size_t not being defined, but I can change it back if you wish, it might just be an Ubuntu bug anyway.

No, that's fine --- if you need it for size_t --- that always gets included for me in <vector> so we don't usually need it.

Maybe then it's an issue of Ubuntu system include files? I got the following error:

g++ -w -Wfatal-errors -I . -isystem lib/eigen_3.2.9 -isystem lib/boost_1.62.0 -isystemlib/cvodes_2.9.0/include -Wall -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DNO_FPRINTF_OUTPUT -pipe -Wno-unused-function -Wno-uninitialized -Wno-c++11-long-long -c -O0 -include stan/math/prim/arr/fun/promote_elements.hpp -o /dev/null test/dummy.cpp In file included from <command-line>:0:0: ./stan/math/prim/arr/fun/promote_elements.hpp: In static member function ‘static std::vector<T> stan::math::promote_elements<std::vector<T>, std::vector<S> >::promote(const std::vector<S>&)’: ./stan/math/prim/arr/fun/promote_elements.hpp:30:14: error: ‘size_t’ was not declared in this scope for (size_t i = 0; i < u.size(); ++i) ^~~~~~ compilation terminated due to -Wfatal-errors. **** clang++ -w -Wfatal-errors -I . -isystem lib/eigen_3.2.9 -isystem lib/boost_1.62.0 -isystemlib/cvodes_2.9.0/include -Wall -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS -DNO_FPRINTF_OUTPUT -pipe -Wno-unused-function -Wno-uninitialized -Wno-c++11-long-long -c -O0 -include stan/math/prim/arr/fun/promote_elements.hpp -o /dev/null test/dummy.cpp In file included from <built-in>:325: In file included from <command line>:5: ./stan/math/prim/arr/fun/promote_elements.hpp:30:14: fatal error: unknown type name 'size_t'; did you mean '__gnu_cxx::size_t'? for (size_t i = 0; i < u.size(); ++i) ^~~~~~ __gnu_cxx::size_t /usr/bin/../lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/ext/new_allocator.h:44:14: note: '__gnu_cxx::size_t' declared here using std::size_t; ^ 1 error generated.

bob-carpenter · 2017-03-19T23:33:57Z

stan/math/prim/mat/fun/to_matrix.hpp

 #include <stan/math/prim/mat/fun/Eigen.hpp>
+#include <Eigen/Dense>


The include on line 7 is all you need---it includes Eigen/Dense and makes sure everything's include din the right order.

bob-carpenter · 2017-03-19T23:34:40Z

stan/math/prim/mat/fun/to_matrix.hpp

 #include <vector>

 namespace stan {
  namespace math {
    /**
     * Returns a matrix with dynamic dimensions constructed from
-     * an Eigen matrix which is either a row vector, column vector, or matrix.
+     * an Eigen matrix which is either
+     * a row vector, column vector, or matrix.


Please include as much on a line as you can.

This sentence cannot fit on 2 lines (of 80 characters), but I changed it now to:
* Returns a matrix with dynamic dimensions constructed from an
* Eigen matrix which is either a row vector, column vector,
* or matrix.

bob-carpenter · 2017-03-19T23:38:20Z

stan/math/prim/mat/fun/to_matrix.hpp

+        Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic>
+          result(rows, cols);
+        for (int i=0, ij=0; i < cols; i++)
+          for (int j=0; j < rows; j++, ij++)
            result(ij) = x[j][i];
        return result;
      } else {
        return Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> (0, 0);


This looks like it wasn't something introduced in this pull request, but I'd like to follow the rules laid out in Martin's awesome Refactoring book on code style, where exceptional cases are called out and return and then the rest of the code continues not in an else-block. So that'd be:

if (rows == 0) return Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> (0, 0);

and then the rest of the code isn't nested. Up to you if you want to change this one. The benefit is that it makes it easier to see the usual line of execution and the exceptional case.

Also, no reason to define a local variable for rows. You can just use x.rows() and x.cols() thoughout.

Since rows and cols are used multiple times (on each iteration of the loop), having them defined locally does speed up thing, doesn't it?

Well, after some tests, it does seem to make a less than 1 second difference for really large matrices (like 10000 x 10000), probably not worth the trouble, and probably make things slower to small matrices.

But (yet another comment, sorry) here we actually don't have

int rows = x.rows() int cols = x.cols()

but actually

int rows = x.size(); int cols = x[0].size();

So in this specific case I think it's worth keeping the local variables to help keep the code readable.

bob-carpenter · 2017-03-19T23:41:02Z

stan/math/prim/mat/fun/to_matrix.hpp

+                       "vector size", size);
+      Eigen::Matrix<double,
+                    Eigen::Dynamic, Eigen::Dynamic> result(m, n);
+      double* datap = result.data();


You can do this without the bare pointer! Just remove datap and in place of it in assignment, use

result(i) = x[i];

Indeed, it was probably slightly faster before check constraints of Eigen was disabled in Stan, but now it might just be the same thing, just making things harder to read.

bob-carpenter · 2017-03-19T23:41:53Z

stan/math/prim/mat/fun/to_matrix.hpp

+     * @param x matrix
+     * @param m rows
+     * @param n columns
+     * @param cm column-major indicator:


Sounds like this shold be a bool given that it has boolean values.

Indeed, but it is OK to consider it True when the user gives something other than 1 or 0 as the value of col_major? (or is it the opposite and C/C++ cast non 0/1 int to bool false?)

bob-carpenter · 2017-03-19T23:42:24Z

stan/math/prim/mat/fun/to_matrix.hpp

+          for (int j=0; j < n; j++, ij++)
+            result(i, j) = x(ij);
+        return result;
+      } else {


Then you can get rid of this test because the bool will have to be 0 or 1. And you can replace if (cm == 1) with just if (cm), though I'd prefer col_major as the variable name so it's easier to understand (no big deal if you like cm better).

ok, by the way, why is m used for the number of rows and n for the number of cols? Isn't this the opposite of the "convention"?

bob-carpenter · 2017-03-19T23:44:01Z

stan/math/prim/mat/fun/to_matrix.hpp

+      boost::math::tools::promote_args<T, double>::type,
+      Eigen::Dynamic, Eigen::Dynamic>
+    to_matrix(const std::vector<T>& x, int m, int n, int cm) {
+      if (cm == 1) {


same comment as in previous

bob-carpenter · 2017-03-19T23:45:51Z

test/unit/math/prim/mat/fun/to_matrix_test.cpp

+  int cols = x.cols();
+  Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic>
+    result(rows, cols);
+  for (int i=0, ij=0; i < rows; i++)


Spaces around = and all other operators, please. You do it in most places, but not everywhere.

Looks like you still missed the i = 0 and ij = 0 case in lines 16 and 17 (and presumably elsewhere).

Yes, I forgot to check the test file

bob-carpenter · 2017-03-20T21:40:22Z

Ah, don't worry about the tests. They're such a mess we're not fixing them.

randommm added 6 commits March 13, 2017 19:38

added to_matrix(matrix, m, n), to_matrix(vector, m, n) and to_matrix(…

981670e

…row_vector, m, n)

added to_matrix(int[], int, int)

adfa418

added to_matrix(matrix, int, int, int), to_matrix(vector, int, int, i…

e3a8e11

…nt) and to_matrix(row_vector, int, int, int)

added to_matrix(real[], int, int, int) and to_matrix(int[], int, int,…

8f502d6

… int)

trailing spaces and cpplint

2e24aa0

fixes

e71b4e3

bob-carpenter requested changes Mar 19, 2017

View reviewed changes

randommm added 3 commits March 20, 2017 14:18

fixes request by @bob-carpenter

ae50ebf

new generalization

716ca6b

forgotten requested changes

e7718d9

bob-carpenter approved these changes Apr 3, 2017

View reviewed changes

bob-carpenter merged commit 1bbd879 into develop Apr 3, 2017

randommm mentioned this pull request Apr 5, 2017

Feature/issue 1452 expose new to matrix functions stan-dev/stan#2264

Merged

3 tasks

randommm deleted the feature/issue-1452-new-to_matrix-signatures branch April 17, 2017 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 1452 new to matrix signatures #508

Feature/issue 1452 new to matrix signatures #508

randommm commented Mar 16, 2017 •

edited by bob-carpenter

Loading

bob-carpenter commented Mar 18, 2017

bob-carpenter left a comment

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

bob-carpenter Mar 20, 2017

randommm Mar 20, 2017

bob-carpenter Mar 19, 2017

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

randommm Mar 20, 2017

randommm Mar 20, 2017

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

bob-carpenter Mar 19, 2017

bob-carpenter Mar 19, 2017

randommm Mar 20, 2017

bob-carpenter Mar 20, 2017

randommm Mar 20, 2017

bob-carpenter commented Mar 20, 2017 via email

		#include <stan/math/prim/mat/fun/Eigen.hpp>
		#include <Eigen/Dense>

Feature/issue 1452 new to matrix signatures #508

Feature/issue 1452 new to matrix signatures #508

Conversation

randommm commented Mar 16, 2017 • edited by bob-carpenter Loading

Submisison Checklist

Summary:

Intended Effect:

How to Verify:

Side Effects:

Documentation:

Reviewer Suggestions:

Copyright and Licensing

bob-carpenter commented Mar 18, 2017

bob-carpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bob-carpenter commented Mar 20, 2017 via email

randommm commented Mar 16, 2017 •

edited by bob-carpenter

Loading