Need help troubleshooting 2D array

[TomBradyGOAT]

I've really been struggling to write a program that finds local peaks/max values in a data file using a 2D array. I keep on getting values that are incredibly small, leading me to believe that the loop isn't even reading in the data of the array.

 #include <iostream>
#include <fstream>
#include <cstring>
#include <cmath>
using namespace std;
int main() {
    //creating the input file
    /*ofstream mydata;
    mydata.open("data.txt");
    mydata<<"hello"<<endl;
    mydata.close();*/
    //open file
    ifstream fin;
    fin.open("data.txt");
    //declare array and variables
    /*int x = 0, y = 0;
    int nrows, ncols;
    double peak[x][y];*/
    //loop to find the peaks
    /*fin>>nrows>>ncols;
    cout<<peak[x][y]<<endl;
    cout<<peak[4][4]<<endl;
    cout<<"end of data"<<endl;*/
    int x, y;
    int nrows = 7, ncols = 7;
    double peak[nrows][ncols];
    for(x = 0; x < nrows; x++)
    {
        if(peak[x][y]>peak[x - 1][y]){
            if(peak[x][y]>peak[x+1][y]){
            for(y = 0; y < ncols; y++)
                {
                    if(peak[x][y]>peak[x][y - 1]){
                        if(peak[x][y]>peak[x][y+1]){

                                cout<<peak[x][y]<<endl;
                            }
                        }
                    }
                }
            }
            cout<<endl;
        }

    fin.close();
    return 0;
}

[Handy Andy]

Hello TomBradyGOAT,

Where is the "data.txt" file or at least a fair sample of what is in it? Can not test the program with out it.

Andy

[Handy Andy]

Hello TomBradyGOAT,

When you look at the array are you wanting the peaks/max values of the entire array? Or just one row at a time? or just one column at a time?

Until you know what you want I do not believe the for loops are working the way you want.

Andy

[poteto]

1) you loop through the x values using the row value when it should be using ncols, vice versa with y, and you should be going in column major in general if you want your code to be 100x more faster (but that is only for simple memory access time, it would be shadowed by how long it takes to print information to cout).
2) you are using uninitialized value y, which "should" be caught by compiler warnings.
3) you are subtracting x by 1 when x is zero on line 29, which "should" be caught by compiler warnings.
4) I don't see where you try to read the file into the array, or generate any sort of data.
5) you are printing the peak every time the peak is overridden, when you probably want just the peak, so use a variable, also the extra endl doesn't make sense.
6) cstring and cmath are unused.
7) are you sure the name "peak" is the best name for an array? considering you are probably gonna want to store your peak variable somewhere?

warnings from web compilers I used:
gcc cpp.sh
(skipping VLA warning)

In function 'int main()': 29:21: warning: array subscript is above array bounds [-Warray-bounds] 29:36: warning: array subscript is above array bounds [-Warray-bounds] 30:38: warning: array subscript is above array bounds [-Warray-bounds]

msvc rextester.com
(modified to not use a VLA)
Suprisingly gcc doesn't find the uninitialized value, but msvc does, while it doesn't find the out of bounds warning...

source_file.cpp(29) : warning C4700: uninitialized local variable 'y' used
/LIBPATH:C:\boost_1_60_0\stage\lib 
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.23506 for x64

If I were you I would first fix the problems I mentioned, and stop using the C interface and use vectors at() function to find future 'out of bound' errors, and treating a vector as a 2d array using 1 block of memory using the math: y*cols + x, which could be hand written or put in a function/macro/class, and this has the benefit of dynamic size which is convenient.

But that doesn't solve your problem since you are reading a 2d array to find a peak of the whole array instead of row by row, so whats the point of a 2d array when you can just read it as a 1d array. You might not even need a for loop if you want to use <algorithm>'s std::max_element function.

[TomBradyGOAT]

First of all, thank you for such a thorough response. I really appreciate it. I should've mentioned in my post that the goal is to find the peak in two dimensions; that is, the value where none of its neighbors (vertically or horizontally) are greater than it. I'll for sure try the suggestions you brought up, but just thought it would be useful to let you know what the actual goal of the assignment is.

[TomBradyGOAT]

Sorry for not responding Andy, I should've answered your question in my response to Poteto.

[poteto]

Oh, I didn't think very hard on that. My bad. I haven't tested it, but if you wrote your code well, you might be able to just check if the x or y coordinate is in bounds before accessing the coordinate and it will just work, but that should only affect the peaks at the edges of the matrix (which will either access data that loops around or random undefined memory).

If you don't have problems with center peaks you can skip this, but the angle I would tackle your problem is by looping through the array normally, then for every index check every neighbor if they have a larger value (with safety), not sure if my way is better or efficient but to me it seems like a direct approach since I think your version is a tiny bit more confusing.

Also it would be nice if you could make a compilable sample showing the contents of the file and the code that loads it, and the received output + expected output.

[TomBradyGOAT]

This is what's in the file:
7 7
542.200000 543.300000 542.300000 543.100000 544.100000 543.600000 543.400000
543.100000 543.400000 546.100000 545.800000 546.800000 547.200000 547.300000
544.100000 544.300000 547.100000 546.900000 548.900000 547.500000 547.700000
544.600000 545.500000 545.600000 547.600000 546.700000 546.300000 545.400000
543.200000 543.900000 544.500000 547.600000 546.500000 545.900000 544.200000
543.400000 544.600000 545.700000 544.900000 544.800000 543.900000 543.300000
543.200000 544.200000 544.800000 544.400000 543.800000 543.100000 542.600000

[E0p9LyTq]

With the contents of your data file, creating and filling a 2D vector:

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

std::vector<std::vector<double>> ReadData(const std::string);

int main()
{
   // create an empty 2D vector
   std::vector<std::vector<double>> dvec;

   // fill the 2D vector from a file
   dvec = ReadData("data.txt");

   // get the number of rows and columns of the filled vector
   unsigned rows = dvec.size();
   unsigned cols = dvec[0].size();

   // let's show we actually filled the 2D vector
   for (unsigned i = 0; i < rows; i++)
   {
      for (unsigned j = 0; j < cols; j++)
      {
         std::cout << dvec[i][j] << ' ';
      }
      std::cout << '\n';
   }

   // now you can manipulate the 2D data
}

std::vector<std::vector<double>> ReadData(const std::string fileName)
{
   std::ifstream ifs(fileName, std::ifstream::in);

   std::string tempString;

   // let's get the dimensions of the 2D array, reading the entire 1st line
   std::getline(ifs, tempString);

   unsigned rows;
   unsigned cols;

   // create a stringstream to parse out the rows and columns
   std::istringstream iss(tempString);

   iss >> rows >> cols;

   // create a 2D vector with the known sizes
   std::vector<std::vector<double>> temp(rows, std::vector<double>(cols));

   // fill the sized 2D vector
   for (unsigned i = 0; i < rows; i++)
   {
      // read each row, creating a stringstream to parse out the data
      std::getline(ifs, tempString);
      std::istringstream iss(tempString);

      for (unsigned j = 0; j < cols; j++)
      {
         // read each individual element from the stringstream
         iss >> temp[i][j];
      }
   }

   // the 2D vector is now sized and filled and a copy returned
   return temp;
}

[TomBradyGOAT]

Thank you so much! I hate to be the bearer of bad news, but I can only use functions that I've learned in class, and we haven't covered vectors or whatever "unsigned is". I'm really sorry for not saying that in my post, and that your work just went to waste.

[E0p9LyTq]

It's not bad new to me, only for you. I gave you a taste of what C++ can do, what you should hope your instructor at some point teaches you.

[TomBradyGOAT]

This is the program:

#include <iostream>
#include <fstream>

using namespace std;
int main() {
//creating the input file
ifstream mydata;
mydata.open("data.txt");
//declare array and variables
int x, y;
int nrows, ncols;
mydata>>nrows>>ncols;
double peak[nrows][ncols];
//loop to find the peaks
for (x = 0; x < nrows; x++) {
for (y = 0; y < ncols; y++) {
if (peak[x + 1][y] > peak[x][y]) {
if (peak[x + 1][y] > peak[x + 2][y]) {
if (peak[x][y + 1] > peak[x][y]) {
if (peak[x][y + 1] > peak[x][y + 2]) {

cout << peak[x][y] << endl;
}
}
}
}
}
cout << endl;
}

mydata.close();
return 0;
}

And this is the output:

5.35144e-304

2.122e-313
5.2405e-304


1.1671e-312

[E0p9LyTq]

If nrows and ncols are not declared as const then double peak[nrows][ncols]; is illegal in modern C++. It doesn't allow variable length arrays (VLAs), the dimension(s) of an array much be known at compile time.

That is why I used std::vector.

Your current compiler may allow VLAs, but that is very bad practice.

A suggestion: forget at this time trying to find your 2D array peaks. Concentrate on getting your file read into your array. You aren't reading the data into your array in your latest attempt.

You are finding the peaks with whatever garbage data the array holds after being created.

And PLEASE use code tags, as you did the first time. You can edit your post and add them.

http://www.cplusplus.com/articles/jEywvCM9/

[TomBradyGOAT]

I tried to have the file read the data into the array with

mydata>>nrows>>ncols

but if that's not what I'm supposed to do then I'll just straight up initialize it as nrows = 7 and ncols = 7. I also just realized how to use the code tags so I'll make sure to do that next time.

[E0p9LyTq]

I tried to have the file read the data into the array with

That just reads the first line of your file, not the rest of the file.

What follows is very crude, without any error checking should reading data from the file fail at any time. This is not something you should do in regular code. This is only for learning purposes.

#include <iostream>
#include <fstream>

int main()
{
   //creating the input file
   std::ifstream mydata("data.txt");

   //declare array and variables
   const int nrows = 7;
   const int ncols = 7;
   double peak[nrows][ncols];

   // read and discard the dimensions on the first line
   int discard;
   mydata >> discard >> discard;

   // now let's read the data for the array
   for (int i = 0; i < nrows; i++)
   {
      for (int j = 0; j < ncols; j++)
      {
         mydata >> peak[i][j];
      }
   }

   // when done reading your file, close it.
   // being neat pays off
   mydata.close();

   // let's make sure the data was read into the array
   // this is just a temporary bit of code to ensure reading the file works,
   // delete when no longer needed
   for (int i = 0; i < nrows; i++)
   {
      for (int j = 0; j < ncols; j++)
      {
         std::cout << peak[i][j] << ' ';
      }
      std::cout << '\n';
   }
   std::cout << '\n';

   //loop to find the peaks
   std::cout << "Peaks: ";

   for (int x = 0; x < nrows; x++)
   {
      for (int y = 0; y < ncols; y++)
      {
         if (peak[x + 1][y] > peak[x][y])
         {
            if (peak[x + 1][y] > peak[x + 2][y])
            {
               if (peak[x][y + 1] > peak[x][y])
               {
                  if (peak[x][y + 1] > peak[x][y + 2])
                  {
                     std::cout << peak[x][y] << ' ';
                  }
               }
            }
         }
      }
   }
   std::cout << '\n';
}

542.2 543.3 542.3 543.1 544.1 543.6 543.4
543.1 543.4 546.1 545.8 546.8 547.2 547.3
544.1 544.3 547.1 546.9 548.9 547.5 547.7
544.6 545.5 545.6 547.6 546.7 546.3 545.4
543.2 543.9 544.5 547.6 546.5 545.9 544.2
543.4 544.6 545.7 544.9 544.8 543.9 543.3
543.2 544.2 544.8 544.4 543.8 543.1 542.6

Peaks: 547.2 544.3 544.5

[TomBradyGOAT]

Thank you so so much for this. I've been stuck on this problem for ages and haven't been sure what to do.

[E0p9LyTq]

You were just forgetting to read the rest of your data. A simple mistake, easy to make.

And not notice when the code isn't doing what you expect.

[TomBradyGOAT]

Ok so apparently the actual peaks are:

Elevation 547.1 at point 3, 3
Elevation 548.9 at point 3, 5
Elevation 545.7 at point 6, 3

And my program is getting the peaks that you got. Not sure why.

[TomBradyGOAT]

nvm, I just set x and y equal to 1 to negate the first row and first column and it worked

[E0p9LyTq]

The rewrite of your logic in the second code for finding the peaks was flawed, as was your original code. Though for different reasons:

   // loop to find the peaks
   std::cout << "Peaks:\n";

   for (int x = 1; x < nrows - 1; x++)
   {
      for (int y = 1; y < ncols - 1; y++)
      {
         if (peak[x][y] > peak[x - 1][y])
         {
            if (peak[x][y] > peak[x + 1][y])
            {
               if (peak[x][y] > peak[x][y - 1])
               {
                  if (peak[x][y] > peak[x][y + 1])
                  {

                     std::cout << peak[x][y] << " at " << (x + 1) << ", " << (y + 1) << '\n';
                  }
               }
            }
         }
      }
   }

Peaks:
547.1 at 3, 3
548.9 at 3, 5
545.7 at 6, 3

Remember that arrays are zero-based, so to get non-programmer understandable indexing you have to add 1.