Sign in to follow this  
Alessandro

C++: problem erasing vector items

Recommended Posts

Alessandro    302
The following code works perfectly when I need to find and remove items in a vector:

[source]
for (int i=0; i<NSIZE; i++)
{
myVec.erase(std::remove_if(myVec.begin(), myVec.end(), myPred(i)), myVec.end());
}

// predicate
struct myPred {
findValue(int i) : id(i) { }
bool operator()(const myVec::myVecLib &obj) {
return obj.ID==id;
}
int id;
};
[/source]

However, I'd like to remove the items that don't match. I thought I could do it easily modifying the predicate function like this (showing only the line I changed):

[source]
...
return obj.ID!=id;
...
[/source]

But in this case the function will actually remove all the items. What am I doing wrong?
Thanks!

Sorry for the code not showing properly, I don't know why but the source=cpp messes things up. Edited by Alessandro

Share this post


Link to post
Share on other sites
SiCrane    11839
Are you doing this in the same loop? First you remove everything that doesn't match 0, which leaves only things that match 0. Then you remove everything that doesn't match 1, which means that you get rid of all the 0s that you left in the first pass.

Share this post


Link to post
Share on other sites
japro    887
Do you really want to call the removal in the loop? Just think about it... if you call "remove this element if its not equal X" for different X the will obviously be no elements left since they would have to be equal to all the different X.

Share this post


Link to post
Share on other sites
Alessandro    302
[quote name='SiCrane' timestamp='1345652792' post='4972257']
Are you doing this in the same loop? First you remove everything that doesn't match 0, which leaves only things that match 0. Then you remove everything that doesn't match 1, which means that you get rid of all the 0s that you left in the first pass.
[/quote]

That's right, I did something stupid. But then what commands can be used that allow to iterate a vector and, at each loop, find a no-match condition and erase it?
I tried using an iterator with the same result...

Share this post


Link to post
Share on other sites
SiCrane    11839
I have no idea what you mean by "find a no-match condition". What exactly do you want to remove from your vector? What exactly do you want left in the vector?

Share this post


Link to post
Share on other sites
rip-off    10976
What are you actually trying to remove? As the others have said, if you want to erase all but a given ID, then use the following:
[code]
int id = /* determine id */;
myVec.erase(std::remove_if(myVec.begin(), myVec.end(), myInversePred(id)), myVec.end());
[/code]

You can also use the std::not1 predicate to invert an existing predicate:
[code]
#include <functional>

int id = /* determine id */;
myVec.erase(std::remove_if(myVec.begin(), myVec.end(), std::not1(myPred(id))), myVec.end());
[/code]

Share this post


Link to post
Share on other sites
Alessandro    302
Sorry for explaining poorly what I intended to do.

Say I have two vector, like:

std::vector<int> someValues;
std::vector<int> someValuesToCheck;

I'd like to remove all the values in someValuesToCheck that are not in someValues.

So that if:

someValues={1,2,5,6,7}
someValuesToCheck={1,3,5,7,8,11}

the resulting someValuesToCheck should have: {1,5,7}

Please tell me if it's not clear. Thanks.

Share this post


Link to post
Share on other sites
Brother Bob    10344
std::set_intersection can do that.
[code]
std::vector<int> results;
std::set_intersection(someValues.begin(), someValues.end(), someValuesToCheck.begin(), someValuesToCheck.end(), std::back_inserter(results));

someValuesToCheck = result;
[/code] Edited by Brother Bob

Share this post


Link to post
Share on other sites
Exessuz    141
try something like this, havent t
[CODE]

std::vector<int> somevalues;
std::vector<int> somevaluesTocheck;
bool status = false;


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

status = false;

for(int j = 0; j < (int)somevalues.size(); j++){

if(somevaluesTocheck[i] == somevalues[j]){

status = true;

}

}


if(!status){
somevaluesTocheck.erase(somevaluesTocheck.begin() + i);
}
}
[/CODE]

Share this post


Link to post
Share on other sites
Alessandro    302
[quote name='Brother Bob' timestamp='1345740634' post='4972644']
std::set_intersection can do that.
[code]
std::vector<int> results;
std::set_intersection(someValues.begin(), someValues.end(), someValuesToCheck.begin(), someValuesToCheck.end(), std::back_inserter(results));

someValuesToCheck = result;
[/code]
[/quote]

That would be very useful. Unfortunately can't use it because I'm using different type of sets (someValues is a int, someValuesToCheck is a structure) and from what I've read in the documentation the back_inserter expects to compare two equal type of sets.
Thanks anyway, this will come useful in other occasions...

Share this post


Link to post
Share on other sites
Alessandro    302
[quote name='rip-off' timestamp='1345740494' post='4972643']
It sounds relatively straightforward, in pseudo code: erase(remove_if(someValuesToCheck, not( ismember(someValues)))).

What part is causing you trouble?
[/quote]

Thanks, I'm going to try this...

Share this post


Link to post
Share on other sites
krippy2k8    646
[CODE]someValues.erase( std::remove_if( someValues.begin(), someValues.end(),
[&]( int val )
{
return (std::find( someValuesToCheck.begin(), someValuesToCheck.end(), val ) == someValuesToCheck.end() );
} ),
someValues.end() );[/CODE]

or

[CODE]
struct myPred
{
myPred(const std::vector<int>& toCheck )
:someValuesToCheck(toCheck)
{}
bool operator()(int val)
{
return (std::find( someValuesToCheck.begin(), someValuesToCheck.end(), val ) == someValuesToCheck.end() );
}
const std::vector<int>& someValuesToCheck;
};
someValues.erase( std::remove_if( someValues.begin(), someValues.end(), myPred(someValuesToCheck) ), someValues.end() );
[/CODE]

(Just realized I got someValues and someValuesToCheck backwards, but you should get the idea) Edited by krippy2k8

Share this post


Link to post
Share on other sites
Alessandro    302
[quote name='rip-off' timestamp='1345671036' post='4972387']
What are you actually trying to remove? As the others have said, if you want to erase all but a given ID, then use the following:
[code]
int id = /* determine id */;
myVec.erase(std::remove_if(myVec.begin(), myVec.end(), myInversePred(id)), myVec.end());
[/code]

You can also use the std::not1 predicate to invert an existing predicate:
[code]
#include <functional>

int id = /* determine id */;
myVec.erase(std::remove_if(myVec.begin(), myVec.end(), std::not1(myPred(id))), myVec.end());
[/code]
[/quote]

I'm trying to use the std::not1 as suggested but I've troubles coding the predicate.

Here is the code:

[source]std::vector<myStruct::myStructLib> myVector;

for (int f=0; f<tempValues.size(); f++)
{
int faceID=tempValues.at(f);
myVector.erase(std::remove_if(myVector.begin(), myVector.end(), std::not1(myPredicate(faceID))), myVector.end());
}

// predicate
struct myPredicate : std::unary_function<int,bool>{
bool operator()(const myStruct::myStructLib &obj) {
return obj.faceID==id;
}
int id;
};[/source]

When I compile I get several errors:
error: no type named 'argument_type' in 'struct myPredicate'
error: no match for call to '(std::unary_negate<myPredicate>) (myStruct::myStructLib&)' Edited by Alessandro

Share this post


Link to post
Share on other sites
Brother Bob    10344
[quote name='Alessandro' timestamp='1345757490' post='4972755']
[quote name='Brother Bob' timestamp='1345740634' post='4972644']
std::set_intersection can do that.
[code]
std::vector<int> results;
std::set_intersection(someValues.begin(), someValues.end(), someValuesToCheck.begin(), someValuesToCheck.end(), std::back_inserter(results));

someValuesToCheck = result;
[/code]
[/quote]

That would be very useful. Unfortunately can't use it because I'm using different type of sets (someValues is a int, someValuesToCheck is a structure) and from what I've read in the documentation the back_inserter expects to compare two equal type of sets.
Thanks anyway, this will come useful in other occasions...
[/quote]
The function allows the two sets to be of different types, as long as each of the two types are less than comparable with itself, and that the type of the first set is less than comparable with the type of the second set (alternatively you provide a predicate to less than compare an element of the first set with an element of the second set). I should have realized that from your first post that the types were different, sorry, so I didn't mention more about that.

That is, if the first set is of type T1, and the second set is of type T2, then you need the following operators:[list=1]
[*]bool operator<(T1, T1)
[*]bool operator<(T2, T2)
[*]bool operator<(T1, T2), or a predicate bool pred(T1, T2)
[/list]
Operator 1 or 2 is the built in int operator (depending on which one is the integer of course), and the other one you have to define as a sorting criteria for your structure. Operator 3 can either be an operator or a predicate you pass a parameter to the set intersect function.

edit: And I should add now that since you probably shouldn't define an arbitrary operator < for your class for 1 or 2 above, and since that you cannot provide that operator as an explicit predicate, you probably shouldn't go this route. It requires an operator that may not be obvious and unambiguous, so you should not define it. Edited by Brother Bob

Share this post


Link to post
Share on other sites
rip-off    10976
@Brother Bob

std::set_intersection requires sorted ranges, which the OP hasn't definitively stated (though the example data is sorted, that could be human convenience).

@Alessandro
The following compiles for me:
[code]
#include <vector>
#include <functional>
#include <algorithm>

struct Example {
int id;
};

struct myPredicate : std::unary_function<Example, bool> {

myPredicate(int id) : id(id) {
}

bool operator()(const Example &obj) const {
return obj.id == id;
}

private:
int id;
};

void foo(std::vector<Example> &examples, const std::vector<int> &identifiers) {
for (unsigned i = 0 ; i < identifiers.size(); ++i) {
int id = identifiers[i];
examples.erase(std::remove_if(examples.begin(), examples.end(), std::not1(myPredicate(id))), examples.end());
}
}
[/code]
Your code was incorrectly using std::unary_function (the first type parameter must match the parameter type of the operator().

Note that there are two simple ways of implementing this (naive) algorithm, essentially swapping the inner and outer loops. These have different performance characteristics, depending on the relative sizes of the two containers. You want the inner loop to be the smaller one, if you can predict this in advance (i.e. if there are always less identifiers than objects, then the identifiers should be the inner loop).

If this is a performance sensitive application and you are frequently attempting to look up an item by ID, you may also want to consider storing the data in a more search friendly manner, e.g. a binary tree or hash map based on identifier. You could also sort the list of identifiers, this way you can quickly binary search for a given element.

If both ranges are sorted, then you can use some of the more advanced C++ algorithms to help you, as Brother Bob is suggesting. You would need, of course, to balance the cost of lookups against the iteration cost (if you frequently visit the entire collection) and the insert/removal cost (depending on frequency of insertion/removal, and in particular if you need random access insertion).

Share this post


Link to post
Share on other sites
Trienco    2555
[code]
for (int f=0; f<tempValues.size(); f++)
{
int faceID=tempValues.at(f);
myVector.erase(std::remove_if(myVector.begin(), myVector.end(), std::not1(myPredicate(faceID))), myVector.end());
}

// predicate
struct myPredicate : std::unary_function<int,bool>{
bool operator()(const myStruct::myStructLib &obj) {
return obj.faceID==id;
}
int id;
};
[/code]

I don't get it. Unless tempValues is empty or contains only one element, you can just as well call myVector.clear() and be done with it. As soon as this for-loop has executed the second time, your vector will always be empty.

At this point it's not a matter of syntax, it's a matter of the whole approach being simply wrong. The only place where a loop should iterate over all tempValues is inside your predicate (and even there I'd prefer find).

[code]
struct IDcontainedIn
{
IDcontainedIn(const Container& c) : container(c) {}

const Container& container;

bool operator()(ObjectType obj)
{
return find(container.begin(), container.end(), obj.id) != container.end();
}
};


myVector.erase(std::remove_if(myVector.begin(), myVector.end(), std::not1(containedIn(tempValues))), myVector.end());
[/code]

Share this post


Link to post
Share on other sites
Alessandro    302
[quote name='Trienco' timestamp='1345784507' post='4972888']
I don't get it. Unless tempValues is empty or contains only one element, you can just as well call myVector.clear() and be done with it. As soon as this for-loop has executed the second time, your vector will always be empty.

At this point it's not a matter of syntax, it's a matter of the whole approach being simply wrong. The only place where a loop should iterate over all tempValues is inside your predicate (and even there I'd prefer find).
[/quote]

That's right, exactly what's happening.

The solution I used is the one suggested by krippy2k8, which uses std::find into the predicate:

[code]
struct myPred
{
myPred(const std::vector<int>& toCheck )
:someValuesToCheck(toCheck)
{}
bool operator()(int val)
{
return (std::find( someValuesToCheck.begin(), someValuesToCheck.end(), val ) == someValuesToCheck.end() );
}
const std::vector<int>& someValuesToCheck;
};
someValues.erase( std::remove_if( someValues.begin(), someValues.end(), myPred(someValuesToCheck) ), someValues.end() );
[/code]

Nonetheless, it was very interesting to read rip-off, Brother Bob and all other folks suggestions. I'm very new to vectors and very confused about those. Hopefully I learned something new today. Edited by Alessandro

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this