0
\$\begingroup\$

I'm currently writing a C++ project with a Qt GUI and would like to read numerical data from user-selected text files through QtFileDialog::getOpenFileName. There are quite a few methods around which use std::vector, but I will be using the arrayfire library and it generally seems to be a bit more convenient to either use C-style arrays instead of std::vector for storing the data in host memory, or to send the data to an arrayfire array directly. This is what I've done here.

The idea is to pull the data into arrayfire arrays that are part of a struct (dataStruct) which is defined as follows:

struct dataStruct{
    af::array d1, d2, d3;
};

To get the filename from the user I call this in the MainWindow instance after a signal from a pushButton in the GUI:

dataStruct dS;
QString tmp =
    QFileDialog::getOpenFileName(this, tr("Open data file"), "C:\\",
                                 tr("Text Files (*.txt)"));
std::string filename = tmp.toStdString();
loadData(filename, dS);

loadData is the function which extracts the data from the text file, and to do so needs to count the number of rows of data. This is done using getNumberOfLines below:

void getNumberOfLines(std::string filename, int &numLines)
{
    std::ifstream in(filename);
    std::string line;

    while(std::getline(in, line))
        numLines  = numLines + 1;
}

And finally, loading the data into the struct arrayfire arrays:

void loadData(std::string filename, structType &dS)
{
    int numLines = 0;
    getNumberOfLines(filename, numLines);

    std::ifstream in(filename);
    std::string line;
    int i = 0;

    af::array data1 = af::constant(0, numLines, f32);
    af::array data2 = af::constant(0, numLines, f32);
    af::array data3 = af::constant(0, numLines, f32);

    while (std::getline(in, line))
    {
        std::istringstream is(line);
        std::string 1_tmp, 2_tmp, 3_tmp;

        is >> 1_tmp >> 2_tmp >> 3_tmp;
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    dS.d1 = data1;
    dS.d2 = data2;
    dS.d3 = data3;
}

I'm not so happy with getNumberOfLines and loadData because I end up reading the file data twice: First to get the number of rows/lines, the second to extract the data. I think it would be much better if this process could be reduced to a single step somehow.

One thing that I would like to implement is a function similar to getNumberOfLines or extension to loadData that allows the user to select data files with an arbitrary number of columns too. This could involve assigning all data to a single arrayfire array with dimensions given by (numRows, numCols, 1, 1) or similar, and then to split this array by columns later on to get the relevant data. Not sure how to implement this yet, so ideas are welcome!

If there are improvements to the code that other people can spot I'd really appreciate it if they could let me know. This will make up a small part of my first "big" project in C++, so please flag up any screamers that I've adopted when handling data or structuring the code.

\$\endgroup\$
  • 1
    \$\begingroup\$ You'll receive better reviews if you show a complete example. For example, I recommend that you show the necessary #include lines, and a main() that shows how to call your function. It's not mandatory, but it really helps! \$\endgroup\$ – Toby Speight Aug 2 '17 at 10:06
  • \$\begingroup\$ @TobySpeight Ok, will see what I can do and give an update later on :) \$\endgroup\$ – user129626 Aug 2 '17 at 10:08
2
\$\begingroup\$

Prefer return over Output parameter

A result as an out parameter seems like a smell to me:

void getNumberOfLines(std::string filename, int &numLines)
{
    std::ifstream in(filename);
    std::string line;

    while(std::getline(in, line))
        numLines  = numLines + 1;
}

Why not return the number of lines as a result of the function?

Also when counting the lines you don't actually need to read the strings out. You just need to find the end of line marker. To avoid the cost of copying and re-sizing lines for data you don't need, you can use the std::istream::ignore() method.

Lets also pass the filename by reference to avoid a copy (copying strings is relatively expensive (its const as you don't modify it)).

std::size_t getNumberOfLines(std::string const& filename)
{
    std::size_t   result = 0;
    std::ifstream in(filename);

    while(in.ignore(std::numeric_limits<streamsize>::max(), '\n') {
        ++result;
    }
    return result;
}

You can now change the call point to:

// From
int numLines = 0;
getNumberOfLines(filename, numLines);

// To
int numLines = getNumberOfLines(filename);

Illegal identifiers:

    std::string 1_tmp, 2_tmp, 3_tmp;

Those are not legal variable names. Not sure how you think this is compiling!

Check stream reads/writes

You should always check the result of a stream input/output operation to make sure it worked. Streams are the one place in the standard where exceptions are turned off so you need to check the error codes (luckily this is easy as the stream used in a boolean context will convert itself into the current error code state).

    // Here you read value from a stream
    // But don't check that it worked.
    is >> 1_tmp >> 2_tmp >> 3_tmp;
    data1(i) = std::stof(1_tmp) * pow(10, -9);
    data2(i) = std::stof(2_tmp);
    data3(i) = std::stof(3_tmp);
    i++;

Should look like this:

    if (is >> 1_tmp >> 2_tmp >> 3_tmp)
    {
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    else
    {
        // If something went wrong with one line.
        // Then do you trust the rest of the stream?
        // I think not. So you should also mark the input stream
        // bad at this point.
        in.setstate(std::ios::failbit);

        // This will cause the outer loop reading the file to stop.
    }

Use the stream read ability

Why read a string when the data is a float?

Why go to the effort of parsing a string when the data could be a float? In your code you are effectively scanning each value twice, which seems like a lot of extra work.

 std::string 1_tmp;
 in >> 1_tmp;
 double 1_val = std::stof(1_tmp);

Instead, use the existing read float capabilities of the stream input operator.

 double 1_val;
 int >> 1_val;

Copy ss Move

This is a copy:

dS.d1 = data1;
dS.d2 = data2;
dS.d3 = data3;

But you never use data[123] again. So you may as well try and move the data.

dS.d1 = std::move(data1);
dS.d2 = std::move(data2);
dS.d3 = std::move(data3);
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for taking the time to go through this. Not had chance to go over it entirely yet, but will be useful. 1_tmp, 2_tmp, etc. were named quickly for the purposes of the post, and aren't how they appear in my code. Oversight on my part there! As for copy vs. move, data[123] are af::arrays so not sure how std::move will cope trying to push them to the GPU. Will be interesting to see. I was hoping that af garbage collection was taking care of data[123] once they go out of scope... \$\endgroup\$ – user129626 Aug 2 '17 at 14:59
  • 3
    \$\begingroup\$ If af::array does not support move semantics then it will be copied. So you loose nothing. But if it supports move then it potetnially becomes much more efficient. data[123] will be destroyed correctly when they go out of scope either way and tidy up so no worries there. Garbage collection in C++! Garbage collection is the poor man's RAII. RAII is fine grained deterministic garbage collection and much superior to a generalized garbage collector you have in other languages. \$\endgroup\$ – Martin York Aug 2 '17 at 15:07
1
\$\begingroup\$

If you want to scan the file just once, just save its contents while accessing it the first time, e.g. in std::vector<std::string>, and don't use the getNumberOfLines() function at all since it's redundant:

std::vector<std::string> loadFileToVector(const std::string& filename)
{
    std::ifstream in(filename);
    std::string line;

    std::vector<std::string> fileLines;

    while(std::getline(in, line))
        fileLines.emplace_back(line);

    return fileLines;
}

You can also get the number of lines that way as it's the size() of the vector now. Of course you have to parse and copy the contents of the vector after that, but accessing this data from vector is waaaaay faster than reading a large file for the second time. Then your loadData() function would look as follows:

void loadData(std::string filename, structType &dS)
{
    std::vector<std::string> fileLines = loadFileToVector(filename);

    af::array data1 = af::constant(0, fileLines.size(), f32);
    af::array data2 = af::constant(0, fileLines.size(), f32);
    af::array data3 = af::constant(0, fileLines.size(), f32);

    int i = 0;
    for (const auto& line : fileLines)
    {
        std::istringstream is(line);
        std::string 1_tmp, 2_tmp, 3_tmp;

        is >> 1_tmp >> 2_tmp >> 3_tmp;
        data1(i) = std::stof(1_tmp) * pow(10, -9);
        data2(i) = std::stof(2_tmp);
        data3(i) = std::stof(3_tmp);
        i++;
    }
    dS.d1 = data1;
    dS.d2 = data2;
    dS.d3 = data3;
}

Also, I don't know your application, but it's worth thinking about more precise name for loadData() as the current name doesn't say much, i.e. what kind of data it loads and for what purpose. If you say it's a larger project, I'd guess this clarification would be helpful, making the code easier to read and understand.

\$\endgroup\$
  • \$\begingroup\$ I've made a couple of edits that don't change much, I hope you don't mind (pending peer review). loadFile changed to loadFileToVector, and there was also an error carried forwards from the code in my question (apologies): wl_tmp, spec_tmp, phase_tmp needed to be changed to 1_tmp, 2_tmp, 3_tmp. \$\endgroup\$ – user129626 Aug 2 '17 at 13:24
  • 1
    \$\begingroup\$ but accessing this data from vector is waaaaay faster than reading a large file for the second time For small files maybe. For large files that may not be so true; you did not reserve space for the vector and thus it will be resized several times with the cost of copying all its elements. \$\endgroup\$ – Martin York Aug 2 '17 at 14:30
  • 1
    \$\begingroup\$ No problem. Also consider what @LokiAstari has written. Maybe in your use case his advice would work better. Measure the performance and decide what's best for you or pick the one that's more appealing for you if the results are similar. \$\endgroup\$ – KjMag Aug 3 '17 at 7:05

Your Answer

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