tags:

views:

785

answers:

8
A: 

You could do it recursively, I guess - something like this (unchecked, but it gives the idea):

// Recursive function
template<typename T>
void REORDER(int oldPosition, vector<T>& vA, 
             const vector<int>& vecNewOrder, vector<bool>& vecVisited)
{
    // Keep a record of the value currently in that position,
    // as well as the position we're moving it to.
    // But don't move it yet, or we'll overwrite whatever's at the next
    // position. Instead, we first move what's at the next position.
    // To guard against loops, we look at vecVisited, and set it to true
    // once we've visited a position.
    T oldVal = vA[oldPosition];
    int newPos = vecNewOrder[oldPosition];
    if (vecVisited[oldPosition])
    {
        // We've hit a loop. Set it and return.
        vA[newPosition] = oldVal;
        return;
    }
    // Guard against loops:
    vecVisited[oldPosition] = true;

    // Recursively re-order the next item in the sequence.
    REORDER(newPos, vA, vecNewOrder, vecVisited);

    // And, after we've set this new value, 
    vA[newPosition] = oldVal;
}

// The "main" function
template<typename T>
void REORDER(vector<T>& vA, const vector<int>& newOrder)
{
    // Initialise vecVisited with false values
    vector<bool> vecVisited(vA.size(), false);

    for (int x = 0; x < vA.size(); x++)
    {
        REORDER(x, vA, newOrder, vecVisited);
    }
}

Of course, you do have the overhead of vecVisited. Thoughts on this approach, anyone?

Smashery
In a first read it seems to me as if this roughly ends up with the same memory and time costs. While you are not copying to another vector, you are copying to just as many local variables as elements in the vector. I would have to work harder on the interpretation to test for correctness.
David Rodríguez - dribeas
Yeah, that was my thought too. In fact, it would probably use up more space because of the stack frames, and take more time because of method-calling overhead. But it was a fun intellectual exercise anyway ;-P
Smashery
A: 

Your code is broken. You cannot assign to vA and you need to use template parameters.

vector<char> REORDER(const vector<char>& vA, const vector<size_t>& vOrder)  
{   
    assert(vA.size() == vOrder.size());  
    vector<char> vCopy(vA.size()); 
    for(int i = 0; i < vOrder.size(); ++i)  
        vCopy[i] = vA[ vOrder[i] ];  
    return vA;
}

The above is slightly more efficient.

dirkgently
Donotalo
... or return a vector.
dirkgently
returning a vector will end up with the result of having to create a copy, which, to my interpretation, is what the OP wants to avoid. Another optimization of your approach would be not creating a vector of size elements but rather reserving memory and using push_backs. This would remove the cost of default constructing all elements before you copy.
David Rodríguez - dribeas
@dirkgently: You're right, vA shouldn't be const.
Marc Eaddy
+2  A: 

If it is ok to modify the ORDER array then an implementation that sorts the ORDER vector and at each sorting operation also swaps the corresponding values vector elements could do the trick, I think.

andreas buykx
Code please! :)
Marc Eaddy
A: 

To iterate through the vector is O(n) operation. Its sorta hard to beat that.

JH
He's talking about space efficiency, not algorithm efficiency.
Justicle
A: 

This should avoid copying the vector:

void REORDER(vector<char>& vA, const vector<size_t>& vOrder)  
{   
    assert(vA.size() == vOrder.size()); 
    for(int i = 0; i < vOrder.size(); ++i)
     if (i < vOrder[i])
      swap(vA[i], vA[vOrder[i]]);
}
Donotalo
I believe this to be broken. Order: [ 2, 0, 1 ], first step will swap first and third element. In the second step i > order[i] and nothing is done, in the third step i > order[i] again and nothing is done.
David Rodríguez - dribeas
This code works with the example but wouldn't work with the vOrder sequence [3 0 1 2]. For i == 0 , 3 and 2 are swapped, then nothing is changed for subsequent i values.
chmike
Thanks for pointing out the mistake. I should've checked more before posting the code. :(
Donotalo
+1  A: 

Never prematurely optimize. Meassure and then determine where you need to optimize and what. You can end with complex code that is hard to maintain and bug-prone in many places where performance is not an issue.

With that being said, do not early pessimize. Without changing the code you can remove half of your copies:

    template <typename T>
    void reorder( std::vector<T> & data, std::vector<std::size_t> const & order )
    {
       std::vector<T> tmp;        // create an empty vector
       tmp.resize( data.size() ); // ensure memory and avoid moves in the vector
       for ( std::size_t i = 0; i < order.size(); ++i ) {
          tmp.push_back( data[order[i]] );
       }
       data.swap( tmp );          // swap vector contents
    }

This code creates and empty (big enough) vector in which a single copy is performed in-order. At the end, the ordered and original vectors are swapped. This will reduce the copies, but still requires extra memory.

If you want to perform the moves in-place, a simple algorithm could be:

template <typename T>
void reorder( std::vector<T> & data, std::vector<std::size_t> const & order )
{
   for ( std::size_t i = 0; i < order.size(); ++i ) {
      std::size_t original = order[i];
      while ( i < original )  {
         original = order[original];
      }
      std::swap( data[i], data[original] );
   }
}

This code should be checked and debugged. In plain words the algorithm in each step positions the element at the i-th position. First we determine where the original element for that position is now placed in the data vector. If the original position has already been touched by the algorithm (it is before the i-th position) then the original element was swapped to order[original] position. Then again, that element can already have been moved...

This algorithm is roughly O(N^2) in the number of integer operations and thus is theoretically worse in performance time as compare to the initial O(N) algorithm. But it can compensate if the N^2 swap operations (worst case) cost less than the N copy operations or if you are really constrained by memory footprint.

David Rodríguez - dribeas
Your algorithm is optimal is vOrder can't be modified or if N is small. Yours may spend more time scanning vOrder but will only swap vA values. Mine swap vOrder and vA values which may end up to be more expensive than to rescan vOrder with small N values.
chmike
+1  A: 

Here is the correct code

void REORDER(vector<char>& vA, vector<size_t>& vOrder)  
{   
    assert(vA.size() == vOrder.size());

    // for all elements to put in place
    for( int i = 0; i < va.size() - 1; ++i )
    { 
        // while the element i is not yet in place 
        while( i != vOrder[i] )
        {
            // swap it with the element at its final place
            int alt = vOrder[i];
            swap( vA[i], vA[alt] );
            swap( vOrder[i], vOrder[alt] );
        }
    }
}

note that you can save one test because if n-1 elements are in place the last nth element is certainly in place.

On exit vA and vOrder are properly ordered.

This algorithm performs at most n-1 swapping because each swap moves the element to its final position. And we'll have to do at most 2N tests on vOrder.

chmike
Your approach of updating the order vector really improves the code, but do you really need the internal while loop? I believe you don't with the following rationale: In the first step of the algorithm you do not need the while loop. It will work in just one step. The fancy part is that you are actually performing a reduction of the original problem.
David Rodríguez - dribeas
@dribeas this is unfortunately not true but what I also thought in the first place. Try with the sequence 2 3 1 0 you'll see that 1 and 0 wont be in the correct place.
chmike
+4  A: 

I improved chmike's algorithm. This function agrees with his for all 11! permutations of (0..10) passed as the reordering vector. Also it doesn't modify reordering vector.

template< class T >
void reorder(vector<T> &v, vector<size_t> const &order )  {   
    for ( int s = 1, d; s < order.size(); ++ s ) {
     for ( d = order[s]; d < s; d = order[d] ) ;
     if ( d == s ) while ( d = order[d], d != s ) swap( v[s], v[d] );
    }
}

Here's an STL style version which I put a bit more effort into. It's about 47% faster (that is, almost twice as fast over (0..10)!) because it does all the swaps as early as possible and then returns. The reorder vector consists of a number of orbits, and each orbit is reordered upon reaching its first member. It's faster when the last few elements do not contain an orbit.

template< typename order_iterator, typename value_iterator >
void reorder( order_iterator order_begin, order_iterator order_end, value_iterator v )  {   
    typedef typename iterator_traits< value_iterator >::value_type value_t;
    typedef typename iterator_traits< order_iterator >::value_type index_t;
    typedef typename iterator_traits< order_iterator >::difference_type diff_t;

    diff_t remaining = order_end - 1 - order_begin;
    for ( index_t s = index_t(), d; remaining > 0; ++ s ) {
     for ( d = order_begin[s]; d > s; d = order_begin[d] ) ;
     if ( d == s ) {
      -- remaining;
      value_t temp = v[s];
      while ( d = order_begin[d], d != s ) {
       swap( temp, v[d] );
       -- remaining;
      }
      v[s] = temp;
     }
    }
}

And finally, just to answer the question once and for all, a variant which does destroy the reorder vector. (It fills it with -1's.) It's about 16% faster than the preceding version. This one uses an ugly typecast, but deal with it. This covers 11! ~= 40 mil permutations of 11 characters in 4.25 seconds, not counting overhead, on my 2.2 GHz laptop.

template< typename order_iterator, typename value_iterator >
void reorder_destructive( order_iterator order_begin, order_iterator order_end, value_iterator v )  {
    typedef typename iterator_traits< value_iterator >::value_type value_t;
    typedef typename iterator_traits< order_iterator >::value_type index_t;
    typedef typename iterator_traits< order_iterator >::difference_type diff_t;

    diff_t remaining = order_end - 1 - order_begin;
    for ( index_t s = index_t(); remaining > 0; ++ s ) {
     index_t d = order_begin[s];
     if ( d == (diff_t) -1 ) continue;
     -- remaining;
     value_t temp = v[s];
     for ( index_t d2; d != s; d = d2 ) {
      swap( temp, v[d] );
      swap( order_begin[d], d2 = (diff_t) -1 );
      -- remaining;
     }
     v[s] = temp;
    }
}
Potatoswatter
This is automatically marked "community wiki" because i made too many edits, and now i don't get reputation for it??
Potatoswatter
@Potatoswatter: yes, that's the way stackoverflow works..., never really understood the reasoning when one guy does the (vast majority of the) editing i. It's like they want to discourage you from improving your answer or something...
Pieter
Complain on meta
jmucchiello