1
\$\begingroup\$

[NOTE] This question can be depreciated in favor of version 0.2.

The purpose of this code is to produce a universe of points, randomly generated around predetermined centroids, provided as a vector of vector. The final product is a file of sample points, to be used for fake data analysis in another program. The objective here was brevity and speed - I feel the code could be vastly concisified, but it is working.

Detailed description of the primary algorithm parameters is in the .h.

// clustergen.h

#include <string>
#include <vector>
#include <iostream>
#include <fstream>
#include <sstream>
#include <random>
#include <chrono>

#ifndef CLUSTERGEN_H
#define CLUSTERGEN_H

// POINT GENERATION - COMMON USER DEFINED VARIABLES
double const PT_BOUND = 10;   // Defines the +/- around a centroid for point generation if UNIFORM
double const PT_SD = 5;       // Defines the stddev around a centroid for point generation if NORMAL

// PRIMARY ALGORITHM
void clustergen(unsigned int k, std::vector<std::vector<double>> &c, std::string file_out, std::string file_rpt, bool csv, bool norm);
// Produces [k] points in [file_out]; all points are separated by line breaks
// If [csv] = 0, dimensions for each point are whitespace-separated
// If [csv] = 1, dimensions for each point are comma-separated
// If [norm] = 0, points will be UNIFORMLY generated around centroids
// If [norm] = 1, points will be NORMALLY generated around centroids
// [c] is a vector of vectors; it's size is the number of centroids
// The first insertion in [c] sets dimensional precedence; dimensional mismatches are always omitted/avoided.

#endif //CLUSTERGEN_H





// clustergen.cpp

#include "clustergen.h"

void clustergen(unsigned int k, std::vector<std::vector<double>> &c, std::string file_out, std::string file_rpt, bool csv, bool norm) {

    std::ofstream fout, rout;
    fout.open(file_out);
    rout.open(file_rpt);

    rout << "CLUSTERGEN STATUS REPORT FOLLOWS...\n";

    unsigned int ct = 0;
    unsigned int cl_ct = 0;

    for (std::vector<std::vector<double>>::iterator c_iter = c.begin(); c_iter != c.end(); ++c_iter) {
        unsigned int subsec;
        std::default_random_engine gen(std::chrono::system_clock::now().time_since_epoch().count());

        (k % c.size() == 0) ? (subsec = (k + ct) / c.size()) : (subsec = 1 + ((k + ct) / c.size()));
        k -= subsec;

        for (unsigned int i = 0; i < subsec; i++) {
            std::vector<double>::iterator d_iter = (*c_iter).begin();
            while (d_iter != (*c_iter).end()) {
                double num;
                if (norm) {
                    std::normal_distribution<double> distr((*d_iter), PT_SD);
                    num = distr(gen);
                } else {
                    std::uniform_real_distribution<double> distr((*d_iter) - PT_BOUND, (*d_iter) + PT_BOUND);
                    num = distr(gen);
                }
                fout << num;
                std::vector<double>::iterator temp_d_iter = d_iter;
                std::vector<std::vector<double>>::iterator temp_c_iter = c_iter;
                if (++temp_d_iter != (*c_iter).end()) {
                    (csv == 0) ? (fout << " ") : (fout << ",");
                } else if ((i != subsec - 1) || (++temp_c_iter != c.end())) {
                    fout << "\n";
                }
                ++d_iter;
            }
            ++ct;
        }
        rout << "\n" << div << " points ";
        (norm) ? (rout << "normally") : (rout << "uniformly");
        rout << " distributed around centroid " << ++cl_ct << " ...";
    }
    rout << "\n\n" << ct << " total points assigned.";
}





// main.cpp
#include "clustergen.h"

int main() {

    std::vector<std::vector<double>> v = {{0,0}, {50,30}, {100,120}};
    clustergen(110, v, "clustergen_out.dat", "clustergen_report.dat", 1, 0);

}
\$\endgroup\$
  • \$\begingroup\$ Please note, this question can be depreciated in favor of version 0.2. \$\endgroup\$ – Miller Aug 11 '17 at 21:02
2
\$\begingroup\$

Overview:

Instantiating a random engine is relatively expensive. Also creating one each loop does not increase your entropy (more likely decreases but I am not a mathamtician so don't homd me to that). BUT it is fine to create it once:

// Do this once at the beginning of the function
std::default_random_engine gen(std::chrono::system_clock::now().time_since_epoch().count());

Modern C++ techniques:

// C++ 03
for (std::vector<std::vector<double>>::iterator c_iter = c.begin(); c_iter != c.end(); ++c_iter)

In C++ 11 we can let the compiler determine the type:

// C++ 11
for (auto c_iter = c.begin(); c_iter != c.end(); ++c_iter)

In C++14 they introduced range based for()

// C++ 14
for (auto& cValue : c) //  Note:: cValue => *c_iter from the above examples.

Looking at the inner loop:

    std::vector<double>::iterator d_iter = (*c_iter).begin();
    while (d_iter != (*c_iter).end()) {
        // STUFF
        ++d_iter;
    }

We replace this while loop with a for loop.

   for (auto d_iter = (*c_iter).begin(); d_iter != (*c_iter).end(); ++d_iter)

Which of course we can then apply all the above transformations on and get the same result:

   for (auto dValue: *c_iter)

We should also note that -> is short hand for (*).

   // eg:
   (*c_iter).begin()

   // written more succinctly as:
   c_iter->begin()

Over Complex

(k % c.size() == 0) ? (subsec = (k + ct) / c.size()) : (subsec = 1 + ((k + ct) / c.size()));

First of that is hard to read. Spread it out:

(k % c.size() == 0)
    ? (subsec = (k + ct) / c.size())
    : (subsec = 1 + ((k + ct) / c.size()));

Second this relies on sideeffects of the expression. This is usually not a good idea. Let expressions by simple (with no side affects). Use the result of the whole thing to do the assignment:

subsec = (k % c.size() == 0)
    ? ((k + ct) / c.size())
    : (1 + ((k + ct) / c.size()));


// Also this:
rout << (norm) ? "normally" : "uniformly";  // from your monstrocity.

I also think I see a common sub expression in there. Extract that and assign it to a meaningful variable name to help document your code:

auto someMeaningfulName = (k + ct) / c.size();
subsec = (k % c.size() == 0)
    ? someMeaningfulName
    : 1 + someMeaningfulName;

OK. Now I can make this even more readable:

auto someMeaningfulName = (k + ct) / c.size();
subsec = someMeaningfulName + ((k % c.size() == 0) ? 0 : 1);

One line per declaration

std::ofstream fout, rout;
fout.open(file_out);
rout.open(file_rpt);

Use one line to declare each variable (and put some more meaningful names on them). Also there is no reason to explicitly call the open() method as this is handeled by the constructor:

std::ofstream fout(file_out);
std::ofstream rout(file_rpt);
\$\endgroup\$
  • 3
    \$\begingroup\$ I think range based for is C++11, and it was not modified until C++17. \$\endgroup\$ – Incomputable Aug 11 '17 at 9:40
  • \$\begingroup\$ This is awesome! Thank you Loki. A note on your "Looking at the inner loop" section: the only reason this was a while() instead of a for() was because d_iter was using a peeking function - I think this was old code and will adjust accordingly. \$\endgroup\$ – Miller Aug 11 '17 at 14:36
  • \$\begingroup\$ Also rout << (norm) ? "normally" : "uniformly"; appears to need parenthesis: rout << ((norm) ? "normally" : "uniformly"); which is interesting - I always thought the ternary operator had the highest precedence? \$\endgroup\$ – Miller Aug 11 '17 at 14:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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