-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix model query count when no candidate result scores are improvements over current result score #350
Conversation
… current result score value
Actually, I think this is still partially incorrect. |
@a1noack I don't think that should matter, since we actually keep track of the number of queries in |
@jinyongyoo But I think |
Okay, after a bit more testing, I'm thinking that I was right the first time. I think the commit I made actually is correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct; good find.
I think it should only matter in cases where the attack ends with an index that it doesn't swap out which is probably relatively rare except maybe with maximizing goal functions.
I think it would be nice to add a comment with a short explanation of why this is necessary; I also think there might be similar issues in most/all of the other SearchMethod
s when the search ends by failing to find any replacements.
@a1noack I also can see why this might be an issue. This isn't an issue for keeping track of the queries we used so far, but it still is an issue for the number queries we report for each goal function result at the end. Thanks for catching it. I think all search methods have this issue and I think there's an easier way to fix this. |
Yeah I think this is a better solution. Good idea |
@jinyongyoo ready to merge? |
@qiyanjun We can merge it, but it'll just fix one search method. What I proposed fixes it for every search method. @a1noack Hey Adam, do you think it's possible for you to fix it this way?
|
Here's a more general fix. It's not beautiful, but I think it should be fine as long as we continue to treat |
Okay, so the Part of the expected output for this test is:
But that part in the new output when I run this test is:
So the new query count is much higher. Which makes sense given the changes I made. Not sure what you want to do about this. A bunch of the tests might be wrong. |
@a1noack Looks good to me! Thank you for catching this mistake and fixing it. I checked out what was happening with the genetic algorithm, and it turns out whenever we perturb a population member/text, we call This reflects that fact that |
It seems that when none of the items in
results
have a higher score than thecur_result.score
, thenum_queries
attribute ofcur_result
exits the while loop in the_perform_search
method with an incorrect value of model queries. I think this fixes the problem.