Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My goals are to make this code:

  • faster
  • more idiomatic C++

What the code does:

  • take an integer command-line argument N
  • make a vector of N random floating point values
  • make a matrix of N*N random floating point values
  • do inner product of vector * matrix
  • write the resulting vector to a file

Currently compiled with: g++ myFile.cpp -std=c++11 -O3

#include <vector>
#include <fstream>

const std::string outputName = "output/coutput.txt"; // the name of the file this program makes

std::ostream& operator<< (std::ostream& os, const std::vector<double>& v)
{ // pretty-print a vector of doubles using operator<<
  for (auto& i : v)
    i == *v.begin() ? os << i : os << " " << i;
  return os;
}

double genRand(void)
{
  return rand() / static_cast<double>(RAND_MAX);
}

int main(int argc, char* argv[])
{
  // file i/o
  std::ofstream result;
  result.open(outputName, std::ios::out);

  srand(1);
  long elems = std::atol(argv[1]); // command line param
  std::vector<double> m(elems * elems); // (elems * elems); // actually this is a vector of length (N*N)
  std::vector<double> a(elems);
  std::vector<double> r(elems, 0); // to store the result

  for (auto i = 0; i < elems; i++) // generate random vector
    a[i] = genRand();

  for (auto i = 0; i < elems * elems; i++) // generate random matrix
    m[i] = genRand();

  for (auto i = 0; i < m.size(); i++) { 
    auto idx = i / a.size();
    r[idx] += a[idx] * m[i];
  }

  result << r << std::endl; // print results
  result.close();
}
share|improve this question
add comment

3 Answers 3

up vote 7 down vote accepted

If you're going to use C++11, you should firstly look at throwing out rand, and using the newer features found in <random>. The setup is a bit more complex, but the distributions are generally much better statistically. Also, instead of generating numbers one at a time, we're going to generate them in a batch:

template <typename Cont = std::vector<double>>
Cont generate_rand(std::size_t n)
{
    Cont results;
    results.reserve(n);

    std::random_device rd;
    std::mt19937 generator(rd());
    std::uniform_real_distribution<> dist;
    auto random = [&]() { return dist(generator); };
    std::generate_n(std::back_inserter(results), n, random);
    return results;
}

With this, we can generate a set of random values into a vector:

// Read into num however many values we want to generate from args
auto vec = generate_rand(num);

Working with an actual matrix is a bit easier, so let's generate that (instead of an n * n length vector):

std::vector<std::vector<double>> matrix;
for(auto i = 0; i < num; ++i) {
    matrix.emplace_back(generate_rand(num));
}

Something to hold our results, with the size correctly reserved:

std::vector<double> result;
result.reserve(num);

The algorithm library already has a function that will do what you want: std::inner_product:

for(auto it = matrix.begin(); it != matrix.end(); ++it) {
    auto v = std::inner_product(std::begin(vec), std::end(vec), std::begin(*it), 0.0);
    result.push_back(v);
}

Alternately, this is the place where a library like Eigen comes in really handy; with the correct includes, this can be written as follows:

#include <Eigen/Core>

typedef Eigen::MatrixXd Dynamic2d;

int main()
{
    // Initialize num from program args
    auto x = Dynamic2d::Random(num, num);
    auto y = Dynamic2d::Random(1, num);
    auto res = y * x;
}

(Note however that Random gives a range between [-1, 1] when called like this, so you'd need to potentially do a bit of work to get that distribution back to [0, 1]). This is likely to give you the maximum performance, as libraries like Eigen have had a lot of time spent on making them highly efficient.

share|improve this answer
    
Nice! About 50% speedup on my machine from your STL suggestions. Hope to try Eigen soon. –  hoosierEE Jul 15 at 3:12
add comment
  • Since outputName is only used in main(), you can just keep it in there.

  • With C++11, you should no longer use rand() because it's considered harmful. The alternative to this is the <random> library.

  • Stay consistent with your use of curly braces. They should also be used for single lines, especially if you end up needing to add more.

  • You should check argc at the very start, just in case a second argument was not given on the command line. If argc is less than 2 (the program name is included), terminate the program.

  • You don't really need to close the file manually; it should do it on its own.

share|improve this answer
    
Thanks, especially for the rand() info. Any tips for making it run faster? –  hoosierEE Jul 15 at 0:35
    
@hoosierEE: I haven't fully studied it yet, but you may try consolidating the generation loops into one, or use an STL function. The loops appear to be the main performance factor. –  Jamal Jul 15 at 0:43
add comment

I must wonder why noone complained about this:

for (auto& i : v)
    i == *v.begin() ? os << i : os << " " << i;

Not only is this very hard to read: The ternary operator is used to return different results conditionally and should not be used to produce different side-effects.

Secondly the test i == *v.begin() may have worked until now but even for random floats there might come a time where the value at begin is in the array multiple times.

Unfortunately there is no standard algorithm that allows to only insert separators between elements (not at the end) so I would probably do something like this:

bool firstElement = true;
for(auto const& i : v) {
    if(!firstElement) {
        os << " ";
    } else
        firstElement = false;
    os << i;
}

Or more in accordance with your code

bool firstElement = true;
for(auto const& i : v) {
    if(!firstElement) {
        os << " " << i;
    } else {
        os << i;
        firstElement = false;
    }
}

If it is acceptable to have an unnecessary whitespace at the end you could also use:

std::copy(v.begin(), v.end(); std::ostream_iterator<double>(os, " "));
share|improve this answer
    
I think the ternary version is easier to read than what you wrote, but good catch with the test not always being valid. And yeah, I didn't want a trailing whitespace. I used this snippet to intersperse commas when printing vectors, where a trailing comma would not do. –  hoosierEE 22 hours ago
    
I would say it depends. I find your ternary version not very readable. It could be improved by better formatting but I'd say that it just feels like a hack to use a ternary operator without using its return value. That is the realm of if and not of the ternary. –  Nobody 16 hours ago
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.