-
Notifications
You must be signed in to change notification settings - Fork 198
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
add simple spatial transform attack #80 #82
base: master
Are you sure you want to change the base?
Conversation
Hi @MasanoriYamada , thanks a lot for the contribution. Sorry that I haven't been able to review it in details. I'll hopefully get back to you this or next week. Apologize and thanks again. |
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.
Thanks a lot for your contribution and the PR. Would you please apply the suggestions?
- please change the VERSION (the last digit)
@@ -0,0 +1,188 @@ | |||
# Copyright (c) 2018-present, Royal Bank of Canada. |
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.
If you are not an RBC employee, please change this to # Copyright (c) 2019-present, Name of the Contributor.
[ref: CONTRIBUTING.md]
@@ -39,6 +39,7 @@ | |||
from .localsearch import LocalSearchAttack | |||
|
|||
from .spatial import SpatialTransformAttack | |||
from .spatial2 import SpatialTransformAttack2 |
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.
Would you please use another name, since we have another SpatialTransformAttack
? Something like SpatialAdversarialTransform
?
max_xent = torch.zeros(n).to(device) | ||
all_correct = torch.ones(n, dtype=torch.bool).to(device) | ||
if hasattr(self.loss_fn, 'reduction'): | ||
self.org_reduction = self.loss_fn.reduction |
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.
would you please change org_reduction
to _org_reduction
?
if random_tries > 0: | ||
# subsampling this list from the grid is a bad idea, instead we | ||
# will randomize each example from the full continuous range | ||
grid = [(42, 42, 42) for _ in range(random_tries)] # dummy list |
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 know the original code is using 42, but is it useful to have this as an input parameter?
from .base import Attack | ||
from .base import LabelMixin | ||
|
||
_MESHGRIDS = {} |
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.
[suggestion] What do you think about adding this as a class attribute, and adding make_meshgird
and transform
as class methods?
Add a simple spatial transform attack was proposed at ICML2019.
I benchmarked with mnist and cifar10.
I didn't include the cifar10 trained model by resnet18 in the repository because it had 43MB (Please comment if you need to.).
It was a stronger attack in mnist than the original paper and a weaker attack in cifar10.
In the original repository, mnist and cifar10 are implemented tensorflow and imagenet is implemented PyTorch.
I used the implementation for imagenet implemented in Pytorch, since the spatial transformation was dependent on tensorflow functions.
These spatial transformations didn't match numerically, but they appear to be nearly identical in appearance.
The original implementation of cifar10 had a standardized layer attached to the model, but my implementation didn't standardize it (standardize discussion: MadryLab/cifar10_challenge#15 (comment)).
The pytest on the GPU had some errors before the change and I ignored them.