How to calculate sin and cos off all vector elements?

Question

My code

#include <iostream>
#include <fstream>
#include <algorithm>
#include <vector>
#include <iterator>
#include <sstream>
#include <cmath>

#define PI 3.14159265

int main(){
    std::ifstream ifs("MFSO7.dat");
    std::string line;

std::vector<float> column1;
std::vector<float> column2;
std::vector<float> column3;
std::vector<float> vkos;
std::vector<float> vsin;

while(std::getline(ifs, line)) // read one line from ifs
{
    std::istringstream iss(line); // access line as a stream
    float item1;
    float item2;
    float item3;

    // Read the items from the line
    iss >> item1 >> item2 >> item3;

    // Add them to the columns.
    column1.push_back(item1);
    column2.push_back(item2);
    column3.push_back(item3);

}

for(int i=0;i<38;i++)
{
vkos[i]=cos(column3[i]* PI/180.0 );
vsin[i]=sin(column3[i]* PI/180.0 );
}

std::cout << vkos[1] << std::endl;

}

Whem I execute the code I got

milenko@milenko-X58-USB3:~/Calibration Files$ ./a1
Segmentation fault (core dumped)

Why?May be I should avoid the loop or...?

1. `std::vector vkos(38, 0.0f);` + `std::vector vsin(38, 0.0f);` 2. Check whether `column3` actually contains at least 38 elements. — Pixelchemist, Dec 10 '15 at 13:30
What did you see when you ran this in a debugger and stepped through your code? (Oh, you have not done that yet?) — abelenky, Dec 10 '15 at 13:38

legends2k · Accepted Answer · 2015-12-10T13:40:03.117

A vector will have some capacity to hold new items. This is different from size, the count of elements that are actually in the vector. Thus a capacity of n that does not mean that it already has n items. A vector would have no items when it just got constructed by the default constructor -- the one with no arguments.

Referring to the i^th element via vector::operator[] is incorrect when i >= n, where n is the size; in your case n is 0. So first you create them by vkos.push_back(cos(value)) instead of directly assigning to the index. On every push_back, the vector's size increases by one.

for(auto angle : column3)
{
    vkos.push_back(cos(angle * PI/180.0));
    vsin.push_back(sin(angle * PI/180.0));
}

if (vkos.size() >= 2)
    cout << vkos[1] << '\n';

`size()` gives the number of elements actually in the `vector` while `capacity()` gives the number of elements that _could be accommodated_ in the `vector`. — legends2k, Dec 10 '15 at 13:35

score 1 · Answer 2 · 2015-12-10T14:36:26.137

1

If you insist on iterating with an index into a vector, you can use:

for (int i = 0; i < column3.size(); ++i) ...

This way, you will at least not try to access at an index that is larger than the current number or elements + 1.

Otherwise, you can try to initialize the vector to have exactly that many values:

std::vector<float> column3(38, 0);

Or if you are using C++11 you could even go for the

for (auto x : column3) ...

edited Dec 10 '15 at 14:36

answered Dec 10 '15 at 13:30

I do not understand how divisibility of 38 by 3 related. Did you notice that OP inserts data into 3 different vectors? – Slava Dec 10 '15 at 14:18
@Slava you are right, i did not read the code carefully enough – Dec 10 '15 at 14:35

score 1 · Answer 3 · answered Dec 10 '15 at 14:21

1

std::valarray is made for that, sin is overloaded with valarray:

vkos = cos(column3 * PI/180.0);
vsin = sin(column3 * PI/180.0);

No need for a loop, that would work.

answered Dec 10 '15 at 14:21

Guillaume Racicot

39,621
9
77
141

score 0 · Answer 4 · answered Dec 10 '15 at 13:52

0

The problem is you are trying to assign to memory you don't have. Why don't you use std::transform? Using standard algorithms will help avoid these types of bugs, can often perform better than hand written loops, and most importantly is easier to read & understand. Also, avoid using #define, prefer constexpr.

constexpr double Pi {3.14159265};
constexpr auto PiDiv180 = Pi / 180;

std::transform(std::cbegin(column3), std::cend(column3), std::back_inserter(vkos), 
              [PiDiv180] (const auto v) { return std::cos(v * PiDiv180); });
std::transform(std::cbegin(column3), std::cend(column3), std::back_inserter(vsin), 
              [PiDiv180] (const auto v) { return std::sin(v * PiDiv180); });

answered Dec 10 '15 at 13:52

Daniel

8,179
6
31
56

It is not a good solution because vector column3 is traversed twice.:) – Vlad from Moscow Dec 10 '15 at 13:54
@VladfromMoscow You cannot say that without any evidence; who knows what optimisations could be made by only having to write to one block of memory inside the loop. And even if there is a small penalty for this, for a small `std::vector` of `floats`, I hardly think this is going to impact overall performance given the file I/O a few lines before. – Daniel Dec 10 '15 at 13:57
There is no any optimization. The both statements will be ecexuted for the same vector column3. – Vlad from Moscow Dec 10 '15 at 14:02
@VladfromMoscow Yes, but one version only writes to one contiguous block of memory in each loop iteration, the other has to jump between two non-contiguous blocks of memory. I don't know if this has better CPU optimisation opportunities.. I'm not a compiler engineer, are you? – Daniel Dec 10 '15 at 14:05
You can test it yourself for big vectors. I am sure that two times traverse the same vector will be less efficient. – Vlad from Moscow Dec 10 '15 at 14:09
@VladfromMoscow wouldn't really prove much for this use-case. Why don't you take a look at [this question](http://stackoverflow.com/questions/8547778/why-is-one-loop-so-much-slower-than-two-loops).. it's not quite the same, but shows how your expectations aren't always met. – Daniel Dec 10 '15 at 14:15
Thanks for the reference. It is interesting. – Vlad from Moscow Dec 10 '15 at 14:25
@VladfromMoscow I did a basic benchmark, and on my machine, the 1-loop version is ~13% faster than the 2-loop version. This seems to be quite consistent with vector size (10 to 100000000) elements. For `double` (rather than `float`s), the difference is even less (~5%). – Daniel Dec 10 '15 at 14:44

Brian Rodriguez · Answer 5 · 2015-12-10T14:27:45.007

It's because you're assigning to elements of vkos and vsin that haven't been constructed yet.

It's best to stick to the STL for these types of loop-based tasks, a lot of errors are taken care of for you. For this particular problem, you're looking for std::transform. The following code has zero overhead compared to your accepted solution:

std::vector<std::pair<float, float>> vkossin{};
std::transform(
    std::begin(column3), std::end(column3), std::back_inserter(vkossin),
    [](float degs) {
        float rads = degs*PI/180.0;
        return std::make_pair(cos(rads), sin(rads));
    });

std::cout << vkossin[1].first << '\n';

std::back_inserter, std::pair, std::make_pair

How to calculate sin and cos off all vector elements?

5 Answers5