Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MasanoriYamada
Copy link

@MasanoriYamada MasanoriYamada commented Jul 21, 2020

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.
image

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.

@gwding
Copy link
Collaborator

gwding commented Jul 28, 2020

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.

Copy link
Collaborator

@masoudhashemi masoudhashemi left a 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.
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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 = {}
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants