Deepfool implementation error

cleverhans-lab / cleverhans

An adversarial example library for constructing attacks, building defenses, and benchmarking both

MIT License

6.2k stars 1.39k forks source link

Deepfool implementation error #1008

Closed VinodS7 closed 5 years ago

VinodS7 commented 5 years ago

Describe the bug The overshoot variable is not used within the while loop in the deepfool_attack function. The perturbation being added at the end after the while loop appears to be redundant code.

Link to lines of code https://github.com/tensorflow/cleverhans/blob/ce6bc7130ab53a0947ddc381599acf749e375dc1/cleverhans/attacks/deep_fool.py#L237

https://github.com/tensorflow/cleverhans/blob/ce6bc7130ab53a0947ddc381599acf749e375dc1/cleverhans/attacks/deep_fool.py#L251

Change suggested In line 237 before adding the perturbation there needs to be a (1+overshoot) term multiplied to the perturbation r_tot.

Line 251 can be removed.

npapernot commented 5 years ago

Thanks! Having discussed this with a colleague, we can see how the argument would work both ways:

in the current implementation, overshoot is only added once the iterative procedure has converged to avoid any numerical instability from preventing the attack from being successful
this overshoot could be added within the loop but that would likely make the iterative procedure less accurate as it tries to find an optimal evasion point.

I would recommend comparing with the procedure described by the authors of deepfool and reopen the issue if they indeed recommend using the overshoot variable within the loop.

VinodS7 commented 5 years ago

I couldn't find any indication in the paper about the overshoot but the paper had accompanying code with it.

https://github.com/LTS4/DeepFool/blob/575f3d847ee65d78c0e3b0306657e3e6d575ba7b/Python/deepfool.py#L75

In this line within the loop they are updating the value with the overshoot term. I am not sure what the actual intent of the authors was and if we can take the code as indication of the intent or whether the code is a bug on their part. I am not sure whether it is worth reopening this issue over that.

npapernot commented 5 years ago

If the implementation provided by the author included in the overshoot term in the loop, it would make sense to update CleverHans' implementation to match the authors' implementation. Feel free to open a PR to do that. Thanks!