Custom mathematical vector class

Question

My first bigger C++ project: a vector class for personal use and statistical computation.

#include <iostream>
#include <vector>
#include <algorithm>
#include <cmath>

template<class T>
class vect
{
    std::vector<T> m;
    size_t s;

public:
    // default constructor
    vect(): m(0), s(0){} 

    // overloaded constructor
    vect(size_t n) :m(n), s(n) {}

    vect(std::vector<T> v) :m(v), s(v.size()) {}

    // copy constructor
    vect(const vect<T>&v): m(v.getData()), s(v.size()){} 

    // destructor
    ~vect(){}

    std::vector<T> getData() const{
        return m;
    }   

    size_t size() const { 
        return s; 
    }

    void addTo(T value){
        m.push_back(value);
        s++;
    }

    void addAt(T value, size_t loc=0){
        s++;
        m.emplace(m.begin()+loc, value);
    }

    void rmFrom(){
        m.pop_back();
        s--;
    }

    void rmAt(size_t loc){
        m.erase(m.begin()+loc);
        s--;
    }

    // returns a sorted copy of the original vector.
    vect<T> sorted(){
        std::vector<T> temp = this->getData();
        std::sort(temp.begin(), temp.end());
        vect<T> v(temp);
        return v;
   }

    double median(){
        // linearly interpolated
        if (s%2==0)
        {
            return ((this->sorted()[(s/2)-1]+this->sorted()[(s/2)])/2);
        }
        else
        {
            return (this->sorted()[(s)/2]);
        }
    }

    double percentile(double p){
        // @p - value between 0 and 1: the percentile
        // rounds to the nearest index and return the corresponding  
        return (this->sorted()[round(s*p)]);
    }

    double sum(){
        // computes the sum of the vectors elements
        double sum = 0;
        size_t l = 0;
        while (l<s) {
            sum+=m[l];
            l++;
        }
        return sum;
    }

    double mean(){
        // computes the mean of the vectors elements
        return this->sum()/s;
    }


    double dot(){
        // computes the inner product of this  vector
        vect<T> temp = *this;
        double dot = 0;
        size_t l = 0;
        while(l<s){
            dot+=pow(temp[l],2);
            l++;
        }
        return dot;
    }

    double dot(vect<T> other){
        // computes the inner product of this and another vector
        vect<T> temp = *this;
        double dot = 0;
        size_t l = 0;
        while(l<s){
            dot+=temp[l]*other[l];
            l++;
        }
        return dot;
    }

    double magnitude(){
        return sqrt(this->dot());
    }

    double manhattan_norm(){
         // computes the manhattan norm
        double n = 0;
        size_t l = 0;
        while(l<s){
            n+=abs(m[l]);
            l++;
        }
        return sqrt(n);
    }

    double p_norm(unsigned int p){
        return pow(this->dot(),1/p);
    }

    vect<T> normalized(){
        // devides the vector by it's eucledian distance
        // which results in a unity vector, meaning
        // the sum the elements of a unity vector add up to one;
        T n = 0;
        vect<T> unity = *this;
        n = unity.magnitude();
        return unity/n;
    }

    vect<T> diff(vect<T> other){
       vect<T> diff;
       return (*this-other);
    }

    double cosine(vect<T> other){
        // returns the cosine of theta of this vector and another
        return this->dot(other)/(this->magnitude()*other.magnitude());
    }

    double angle(vect<T> other){
        return acos(this->cosine(other))*(180/3.14159265);
    }

    bool is_perpendicular(vect<T> other){
        return (this->dot(other)==0);
    }

    vect<T> direction_cosine(){
        vect<T> direction;
        double norm = this->norm();
        double cosine = 0;
        double theta = 0;
        for(size_t i = 0; i < s; i++){
            cosine = m[i]/norm;
            theta = acos(cosine);
            direction.addTo(theta);
        }
        return direction;
    }

    vect<T> parallel_comp(vect<T> other){
        // projection of other onto *this
        vect<T> temp = *this;
        return (temp.dot(other)/temp.dot(temp))*temp;
    }

    vect<T> perpendicular_comp(vect<T> other){
        return other-parallel_comp(other);
    }

    double parallel_magnitude(vect<T> other){
        // returns the the projections size 
        return this->parallel_comp(other).magnitude();
    }

    double error_magnitude(vect<T> other){
        // returns the the parallel components size relative to it's own
        return this->perpendicular_comp(other).magnitude();
    }

   // overloaded operators
   vect<T>& operator=(const vect<T>& other){
       if (this!=&other)
       {
            m = other.getData();
            s = other.size();
       }
       return *this;
    }
    T& operator[] (size_t i) {
        return m[i]; 
    }
    const T& operator[] (size_t i) const { 
        return m[i]; 
    }

};


template<class T>
std::ostream& operator<<(std::ostream &os, const vect<T>&v){
    for(int i = 0; i < v.size(); i++){
        os << v.getData()[i] << " ";
    }
    return os;
}

template<class T>
std::istream& operator>>(std::istream& is, vect<T>& v){
    T value;
    is >> value;
    v.addTo(value);
    return is;
}


template<class T>
vect<T>& operator+=(vect<T>& a, const vect<T>& b)
{  
    for(size_t i=0; i<a.size(); ++i) 
        a[i]+=b[i]; 
    return a; 
}

template<class T>
vect<T> operator+(const vect<T>& a, const vect<T>& b)
{ 
    vect<T> z(a.size()); 
    for(size_t i=0; i<a.size(); ++i) 
       z[i] = a[i]+b[i]; 
    return z; 
}

Plus many more overloaded arithmetic operators which are all like the last two ones.

I know it's nothing fancy but I am always looking for ways to make things better.

Loki Astari · Answer 1 · 2014-09-26 20:03:17Z

C++14

Its 2014 most modern compilers now support C++14 so you should use it. This code is still very C++03. For this class this simply means adding move semantics (and nothrow on swap).

To add move semantics you need to add a move constructor and move assignment operator.

vect(vect&& other);
vect& operator=(vect&& other);

Potentially there are a couple of places where you can use range based for operator.

General

This seems redundant (and thus dangerous).

size_t s;

The size is already stored as part of the the other member m;

Const Correctness

None of these functions

double sum();
double mean();
double dot();

modify the state of your vector. So you should mark them as const members.

double sum()  const;
double mean() const;
double dot()  const;

This allows you pass your vect to a function as a const reference and still call these non mutating functions.

Efficiency

You may want to cache the sum in a mutable member. Invalidate it if the vector is mutated. There is no point re-computing this value if the vector has not changed.

double sum(){
    // computes the sum of the vectors elements
    double sum = 0;
    size_t l = 0;
    while (l<s) {
        sum+=m[l];
        l++;
    }
    return sum;
}

Also prefer to use some of the algorithms that are built for you when you can.

 sum = std::accumulate(m.begin(), m.end());

Copy and Swap

This is an old way of doing this:

// overloaded operators
vect<T>& operator=(const vect<T>& other){
   if (this!=&other)
   {
        m = other.getData();
        s = other.size();
   }
   return *this;
}

The more modern way is to use the copy and swap idium.

// overloaded operators 
vect<T>& operator=(vect<T> other){  // Notice the pass by value to get the copy.

   other.swap(*this);    // Swap the content of this object.
                         // With the copy you just created. This updates it
                         // in an exception safe way,
   return *this;         // return yourself.
}                        // Let the destructor of other cleanup your old state.

void swap(vect<T>& other) throws() { // use C++11 nothrow if you upgrade to C++11
   std::swap(this->m, other.m);
   std::swap(this->s, other.s);
}

The `X` operators can be written in terms of the `X=` operator.

Example:
Define the + operator in terms of the += operator.

template<class T>
vect<T> operator+(const vect<T>& a, const vect<T>& b)
{
    vect<T>  result(a);  // Make a copy.
    result += b;         
    return result;
}

Once you studied that and see it does the same. There is a small optimization.

template<class T>
vect<T> operator+(const vect<T> copy, const vect<T>& b)
{
    copy += b;       // Notice the copy is passed by value.
                     // So there is an implicit copy
                     // So you don't need the manual copy inside the code.  
    return copy;
}

Your streaming operations should be symmetrical and distinguishable.

The output operator dumps the whole vector:

template<class T>
std::ostream& operator<<(std::ostream &os, const vect<T>&v){
    for(int i = 0; i < v.size(); i++){
        os << v.getData()[i] << " ";
    }
    return os;
}

But the read operator only reads a single value into the vector (so its not symmetrical).

template<class T>
std::istream& operator>>(std::istream& is, vect<T>& v){
    T value;
    is >> value;    // Also note. That if the read fails.
    v.addTo(value); // you are still adding it to the vector.
                    // You should probably test to make sure the read works.
                    // if (is >> value) {v.addTo(value);}
    return is;
}

Personally I would make it dump the data in a way that makes it obvious of the start/end point (or) you can prefix the dump with a count and then the values.

template<class T>
std::ostream& operator<<(std::ostream &os, const vect<T>&v){
    os << v.size() << ": ";
    for(int i = 0; i < v.size(); i++){
        os << v.getData()[i] << " ";
    }
    return os;
}

// Note: This does not work for T == std::string as it does not
//       have symmetric input output operators. But all other normal types do.
template<class T>
std::istream& operator>>(std::istream& is, vect<T>& v){
    std::size  s       = 0;
    char       marker  = 'B';

    if (is >> s >> marker)
    {
        if (marker != ':')
        {   // The mark is not what we expect.
            // So mark the stream as bad so that processing stops.
            is.setstate(std::ios::failbit);
        }
        else
        {
            T  value;
            for(;s > 0 && (is >> value);--s)
            {
                // Have values and successful read.
                v.addTo(value);
            }
        }
    }
    return is;
}

Some minor remarks: "range based for operator" looks like a typo (?). "You may want to cache the sum in a mutable member." Might violate Single Responsibility Principle. "Also prefer to use some of the algorithms" Somehow was swallowed by a code block. accumulate requires a third argument. "The more modern way is to use the copy and swap idium." Typo, and don't let Howard Hinnant hear this ;) "There is a small optimization" First parameter must not be const. — dyp, 6 hours ago

janos · Answer 2 · 2014-09-26 18:51:29Z

Naming

Your "vector" is not really like std::vector, but vect sounds like it. It would be more intuitive to rename it some thing different, for example vectorstats, or just vstats, or something.

The m and s variables are terrible.

size

I don't really see the point of the s variable. Why do you want to count the size yourself? Why not use m.size()? That would be a lot less error-prone, because as a general rule, the less lines of code you write yourself the better.

sum

It would be more natural to use the iterator pattern, shorter and better:

double sum() {
    double sum = 0;
    for (std::vector<T>::const_iterator it = m.begin(); it != m.end(); ++it) {
        sum += *it;
    }
    return sum;
}

dot

The temp variable is completely pointless in this method:

double dot(){
    // computes the inner product of this  vector
    vect<T> temp = *this;
    double dot = 0;
    size_t l = 0;
    while(l<s){
        dot+=pow(temp[l],2);
        l++;
    }
    return dot;
}

You could rewrite as:

double dot() {
    double dot = 0;
    for (std::vector<T>::const_iterator it = m.begin(); it != m.end(); ++it) {
        dot += pow(*it, 2);
    }
    return dot;
}

The same is true for the overloaded version of this method.

Excessive parentheses

The too many parantheses are seriously hurting readability here:

return ((this->sorted()[(s/2)-1]+this->sorted()[(s/2)])/2);

And the lack of spaces around operators don't help!

This should have been:

return (this->sorted()[s / 2 - 1] + this->sorted()[s / 2]) / 2;

Or actually, it's wasteful to sort the vector twice, better sort once and save in a temp variable:

std::vector<T> temp = this->sorted();
return (temp[s / 2 - 1] + temp[s / 2]) / 2;

Now that I look at this cleaner version, it's clear that this won't work if the vector is empty, because if s == 0 this will end up referencing temp[-1]. This was not so easy to see in the original version, now it's obvious.

This was just the worst example I found, but in many many places you use far more parentheses than you need. I suggest to review the entire code and trim a little bit.

Also try to put spaces around operators like I did in this example.

Placement of curly braces

You're placing curly braces inconsistently. Sometimes you use put the opening brace on the same line as the statement, like this:

double sum(){
    // ...
    while (l<s) {
        // ...
    }
}

Other times you place opening brace on the next line, like this:

if (s%2==0)
{
    // ...
}
else
{
    // ...
}

I suggest to pick either of these styles and stick to it. (I prefer the first.)

Jamal · Answer 3 · 2014-09-26 18:27:23Z

The class name should be capitalized so that it's easily distinguished from functions and variables.
m is a very undescriptive name as it's only one letter. Write out the entire name instead of giving a random letter or some abbreviation. It's especially important to do this so that others can know what this vector is storing.
You don't need s; the size is already known to the vector structure. Just access its size via the size() member function.
If you're just summing the values in a vector, you can use std::accumulate():
```
return std::accumulate(m.cbegin(), m.cend(), 0);
```
(That would be the entire body of sum().)

glampert · Answer 4 · 2014-09-26 18:37:49Z

You don't need an empty destructor. Since it is a no-op. You can leave it out and the compiler will provide a default for you.
The default constructor, vect() should not init m with 0.
The copy constructor, vect(const vect<T>&v) is not needed either. The compiler will generate the exact same code for you if you don't provide it.
I don't really see the need for the s variable here. It is just keeping a copy of m.size(). Why not just use std::vector::size() for that? It is a bunch of extra work to keep that var up-to-date.
std::vector<T> getData() const might be more efficient if returning by reference. If the intended use for this method is to allow read-only access to the internal vector, then there is no reason to return by copy. Return by const reference: const std::vector<T> & getData() const.
rmFrom() is a very unclear name. rm is short for remove? Then just call it remove.
The methods that add data to the vector, such as addTo()/addAt() should be provided in two flavors: one taking a const T& and one taking a T&&. If you refer to std::vector:push_back() you will see that it provides these two overloads to optimize for move semantics (move semantics will require a C++11 capable compiler).
You should probably also provide a move constructor and a move assignment operator for your vect. See the rule of three/five/zero.
Unneeded temps inside dot(). Why are you creating temporary copies of the vector inside both dot() methods? That is nonsense since both function only read from the vector.
Your code is not fully const correct. There are several methods that need to be made const.
In vect<T> diff(vect<T> other) there is a local variable inside (also named diff) that is never used.
You are using the <cmath> functions. So functions like sin, cos, acos, etc should be prefixed with std::. E.g.: std::acos(). That would be the correct form.
The array access operators [] would benefit from some runtime bounds checking. I suggest adding asserts to those. E.g.: assert(i < m.size());

vnp · Answer 5 · 2014-09-26 19:13:46Z

Few missing poins.

STL provides convenient algorithms for pretty much all loops in your code. Besides std::accumulate, take a look at std::inner_product and std::transform algorithms.
I question a mathematical validity of p_norm. It calculates \$ (\sum {m_i}^2)^\frac{1}{p}\$. Shouldn't it be \$ (\sum {m_i}^p)^\frac{1}{p}\$ instead?
Certain operations (such as operator+= and dot) only make sense for vectors of the same dimension. Depending on the size of your vectors you'd be getting either a not very meaningful result or an exception. Doesn't seem right. I don't know your use case; I'd recommend to address the problem anyway.

asked	today
viewed	207 times
active	today

current community

your communities

more stack exchange communities

Custom mathematical vector class

5 Answers 5

C++14

General

Const Correctness

Efficiency

Copy and Swap

The `X` operators can be written in terms of the `X=` operator.

Your streaming operations should be symmetrical and distinguishable.

Naming

size

sum

dot

Excessive parentheses

Placement of curly braces

Your Answer

Not the answer you're looking for? Browse other questions tagged c++ computational-geometry or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Custom mathematical vector class

5 Answers 5

C++14

General

Const Correctness

Efficiency

Copy and Swap

The X operators can be written in terms of the X= operator.

Your streaming operations should be symmetrical and distinguishable.

Naming

size

sum

dot

Excessive parentheses

Placement of curly braces

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c++ computational-geometry or ask your own question.

Related

Hot Network Questions

The `X` operators can be written in terms of the `X=` operator.