Returning Static Reference

[shs2017]

Hi I'm new this forum. I just joined because of an issue I couldn't figure out. I'm developing a matrix class for a Linear Regression Project I doing in my free time. In my main file I try to multiply two matrix classes I made.

#include <iostream>
#include <windows.h>
#include "LinearRegression.h"
#include "plot.h"

using namespace std;

/*

Test Data 1:
[1,		5],
[6,		4],
[8,		9],
[0,		10],
[10.3,	4.1],

Test Data 2:
[1,	2,	3,	4,	5]
[7,	8,	9,	1,	3]


*/

int main(int argc, char* argv[])
{
	matrix<float> myMatrix1(argv[1]);
	matrix<float> myMatrix2(argv[2]);
	matrix<float> myMatrix3;

	myMatrix3 = myMatrix1.multiply(myMatrix2);   //Here's where things go wrong
	cout << myMatrix3.getLength();
	cout << myMatrix3.getHeigth();
}

The issue here is that everything works fine except for two elements of myMatrix3 are changed from the expected value to a wrong one. To clarify here is some more code.

template <typename T>
class matrix
{
	public:	
		matrix();
		matrix(char* filename);
		T getElement(unsigned int colNum, unsigned int rowNum);
		vector<T> getRow(unsigned int row); //Matrix starts at natural numbers (e.g. 1, 2, 3, 4,...)
		vector<T> getColumn(unsigned int column); //Matrix starts at natural numbers (e.g. 1, 2, 3, 4,...)
		T sumColumn(unsigned int column);
		int getLength();
		int getHeigth();
		vector<int> getDimensions();

		void setElement(unsigned int colNum, unsigned int rowNum, T myData);
		void setRow(unsigned int rowNum, vector<T> row);
		void setColumn(unsigned int colNum, vector<T> column);
		void setMatrix(const matrix<T> &otherMatrix);

		vector<T> operator()(unsigned int column, unsigned int row);
		matrix<T> operator+(const matrix<T> &other);
		matrix<T> operator-(const matrix<T> &other);
		matrix<T> operator*(matrix<T> &other);
		matrix<T> operator/(const matrix<T> &other);
		matrix<T> operator%(const matrix<T> &other);
		void operator=(const matrix<T> &other);
		
		void printMatrix();
		void printMatrix(const char* delim);
		void printMatrix(vector<T> thisVector);
		void printMatrix(vector<T> thisVector, const char* delim);
		void printMatrix(unsigned int column, unsigned int row);
		void printMatrix(unsigned int column, unsigned int row, const char* delim);
		
		matrix<T> add(const matrix &other);
		matrix<T>& multiply(matrix &other);
		matrix<T> subtract(const matrix &other);
		matrix<T> divide(const matrix &other);
		matrix<T> modulus(const matrix &other);
		T sum(vector<T> &other);
		
	protected:
		vector<T> data;
		ifstream file;
		unsigned int columns;      //This value
		unsigned int rows;         //and this value are the ones that are different
		vector<T> multiply_vector(vector<T> vec1, vector<T> vec2);
		void failure();
};

When I mean different from expected I mean that I set the variables in this function

template <typename T>
matrix<T>& matrix<T>::multiply(matrix &other)
{
	if(rows != other.columns)
	{
		failure();
	}
	
	else
	{
		static matrix<T> tmpMatrix;
		for(int j=1; j<=rows; j++)
		{
			for(int i=1; i<=rows; i++)
			{
				vector<T> tmp = multiply_vector(getRow(j), other.getColumn(i));
				tmpMatrix.data.push_back(sum(tmp));
			}
		}
		
		tmpMatrix.rows=rows;                    //I set them here
		tmpMatrix.columns=other.columns;        //and here
		tmpMatrix.printMatrix();
		return tmpMatrix;
	}
}

They have both values of 5 but when I return and print out the values for them in the "alias" of tmpMatrix called myMatrix3 they are both 0. When I use gdb to try to come to sense with this it says that the address of tmpMatrix.rows is 3 and tmpMatrix.columns is 4. When I continue and get back to the main function after multiply is called myMatrix.rows address is 5 and myMatrix.columns address is 6. Why is it that I try to set the address of them the same they are different.

Thanks for your time,
Jackson

[ne555]

http://www.eelis.net/iso-c++/testcase.xhtml (point 6)

> in the "alias" of tmpMatrix called myMatrix3
myMatrix3 is not an alias for tmpMatrix

	matrix<float> myMatrix3; //an object
	myMatrix3 = myMatrix1.multiply(myMatrix2); //assignation 

given that you have coded your own assignment operator, we'll need to see it.

There would be no need to code your own assignment operator (and copy constructor) if you didn't have a file as a data member. ¿why do you have that?


In your multiply() function, you are adding items with `push_back()'
Consider what would happen if you call this function a second time.

Also, you may encapsulate better by using the member function already provided. Like setElement()


> if(rows != other.columns)
That's backwards
C_{jk} = A_{jr} B_{rk}
note that 'r' represents the columns of A, and the rows of B


By the way, take a look at `std::valarray'

[shs2017]

Never mind I got it working. Could you clarify on how if(rows != other.columns) is backwards. Also valarray probably would have been a better choice but I'm just doing this for fun so I probably won't change it. Anyways thanks for your help.

[ne555]

> how if(rows != other.columns) is backwards
You've got that as a failure condition, however you are rejecting good ones and accepting bad ones.

By instance
A_{3x2} (three rows, two columns)
B_{5x3}
C_{2x4}

A*B is illegal, but you try to do it
A*C is fine, but you reject it.


> Never mind I got it working
I suggest that you upload your working code so we can review it.

[shs2017]

Here is the link to a github I made for this https://github.com/shs2017/Machine-Learning . All help is appreciated. Let me know if you think of any better ways to do things. Just realise I've been coding this and my freetime so It might not be exactly "professional". I've changed my mind and am going to change a lot of things around such as changing std::vector to std::valarray.

Anyways, thanks for the help,
Jackson

[ne555]

> static matrix<T> returnValue(rows, columns);
¿may you explain why are you doing that?


> matrix<T>& operator^(matrix<T> &other);
that's a bad idea.
^ has low precedence, by instance A+B*C^-1 would be interpreted as (A + (B*C))^-1


> matrix<T>& operator+(matrix<T> &other);
read about const correctness
matrix<T> operator+(const matrix<T> &other) const;


> while(!file.eof())
don't loop on eof, but on the reading operation
while( getline(file, thedata) )


By the way, ¿could you provide example input files?

[shs2017]

>

static matrix<T> returnValue(rows, columns);

¿may you explain why are you doing that?

Sure, I'm doing this because when I didn't the compiler couldn't pass the variable without it being static. I'm newish to c++ so could you clarify why this would happen. Also when you return it by reference, which is necessary if this library could ever scale up to bigger operations, you would have to either make the variable static or put it on the heap.

>

matrix<T>& operator+(matrix<T> &other);

Okay, I'll change this.

>

while(!file.eof())

don't loop on eof, but on the reading operation

while( getline(file, thedata) )

I'll also change this.

>

By the way, ¿could you provide example input files?

Sure. I just uploaded them to github. To test them pass data.csv and data1.csv as arguments when running the program.

> Also valarray couldn't be used as a result of it being static in size rather than the vector which is more dynamic.

[ne555]

> when I didn't the compiler couldn't pass the variable without it being static.
If you were having errors then show the messages.
Before you had an `ifstream' member that may the class non-copyable


> Also when you return it by reference,
> which is necessary if this library could ever scale up to bigger operations
you are confused.
Yes, it is true that you should avoid copying big data, however you were not avoiding it.

myMatrix3 = myMatrix1.multiply(myMatrix2);
there you are copying. It doesn't matter that you are "returning a reference", each object have their own copy of the data.

It is better to return by value
http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/ (sadly it is not currently working)
C++11 Style (move semantics) http://www.youtube.com/watch?v=0iWb_qi2-uI (about min 37)


> you would have to either make the variable static
¿do you know what does that mean? The variable would be constructed just one time, in all the other invocations of that function it would use the same variable.
So if you do A+B your `tmpMatrix' would have the size of A. ¿what if later you try C+D, where the size of C is different?


> Also valarray couldn't be used as a result of it being static in size
that's a shame, although you can "resize" a valarray
however ¿why do you need such requirement? A matrix of different size is another matrix

[shs2017]

> Compiler output of non-working code. Non-working is now uploaded on github).

9	0	C:\Program Files (x86)\Dev-Cpp\LinearRegression.h	In file included from LinearRegression.h
4		C:\Program Files (x86)\Dev-Cpp\main.cpp	                 from main.cpp
C:\Program Files (x86)\Dev-Cpp\matrix.h	In instantiation of 'matrix<T>& matrix<T>::operator*(const matrix<T>&) [with T = float]':
45	27	C:\Program Files (x86)\Dev-Cpp\main.cpp	required from here
434	34	C:\Program Files (x86)\Dev-Cpp\matrix.h	[Error] invalid initialization of non-const reference of type 'matrix<float>&' from an rvalue of type 'matrix<float>'
C:\Program Files (x86)\Dev-Cpp\matrix.h	In instantiation of 'matrix<T> matrix<T>::multiply(const matrix<T>&) [with T = float]':
434	34	C:\Program Files (x86)\Dev-Cpp\matrix.h	required from 'matrix<T>& matrix<T>::operator*(const matrix<T>&) [with T = float]'
45	27	C:\Program Files (x86)\Dev-Cpp\main.cpp	required from here
536	66	C:\Program Files (x86)\Dev-Cpp\matrix.h	[Error] passing 'const matrix<float>' as 'this' argument of 'std::vector<T> matrix<T>::getColumn(unsigned int) [with T = float]' discards qualifiers [-fpermissive]
28		C:\Program Files (x86)\Dev-Cpp\Makefile.win	recipe for target 'main.o' failed

> Let me know if I implemented the code the right way (I added the move constructor and move assignment operator) and what I need to fix. BTW the function I changed to not use the static variable is matrix<T>::multiply function that way the errors weren't pages worth of redundant errors.

> I did some research on valarray. I heard it wasn't as efficient as vector. Is that true? Also what is the advantage of using it.

Thanks,
Jackson

[ne555]

> [Error] invalid initialization of non-const reference of type 'matrix<float>&'
> from an rvalue of type 'matrix<float>'
operator* returns a reference
multiply() returns a temporary
you can't bind a reference to a temporary, change `operator*'

matrix<T> //value not reference
matrix<T>::operator*(const matrix<T> &other) const //const correctness
{
   return this->multiply(other);
}

> [Error] std::vector<T> matrix<T>::getColumn(unsigned int) discards qualifiers
const-correctness, if your member function does not change the state of the caller object, declare it as const
std::vector<T> matrix<T>::getColumn(unsigned int) const


By the way
> here I'm quoting you
this is my voice now