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

Fix min_epsilon of BoundaryAttack to receive a non-negative value #1262

Merged

Conversation

kaitokishi
Copy link
Contributor

Signed-off-by: kaitokishi kishi.kaito@fujitsu.com

Description

Passing a positive float value into min_epsilon of BoundaryAttack raises Exception. This is because _check_params() raises ValueError even if self.min_epsilon is positive float. This PR is to fix this bug.

Also, this PR makes it simple to use min_epsilon by avoiding to use None.

Fixes #1258

Type of change

Please check all relevant options.

  • Improvement (non-breaking)
  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Testing

  • Below code
attack = BoundaryAttack(model, batch_size=100, targeted=False, max_iter=20, epsilon=0.1, min_epsilon=0.02)
x_adv = attack.generate(x=x)

works.

Test Configuration:

  • OS: Ubuntu 18.04.5 LTS
  • Python version: 3.6.9
  • ART version or commit number: 1.7.0
  • TensorFlow version: 2.3.0

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Signed-off-by: kaitokishi <kishi.kaito@fujitsu.com>
@kaitokishi
Copy link
Contributor Author

I would like to make a pull request to dev_1.7.2 but I cannot select the branch because there is no dev_1.7.2 branch. Thus, I selected dev_1.8.0.

@beat-buesser beat-buesser self-assigned this Aug 11, 2021
@beat-buesser beat-buesser self-requested a review August 11, 2021 11:06
@beat-buesser beat-buesser added bug Something isn't working improvement Improve implementation labels Aug 11, 2021
@beat-buesser beat-buesser added this to Pull request open in ART 1.7.2 via automation Aug 11, 2021
@beat-buesser beat-buesser added this to the ART 1.7.2 milestone Aug 11, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

Codecov Report

Merging #1262 (f55dc71) into dev_1.7.2 (4b2745e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           dev_1.7.2    #1262   +/-   ##
==========================================
  Coverage      43.42%   43.42%           
==========================================
  Files            231      231           
  Lines          21201    21202    +1     
  Branches        3956     3956           
==========================================
+ Hits            9207     9208    +1     
  Misses         10852    10852           
  Partials        1142     1142           
Impacted Files Coverage Δ
art/attacks/evasion/boundary.py 76.00% <0.00%> (ø)
art/estimators/classification/pytorch.py 72.43% <0.00%> (+0.06%) ⬆️

@beat-buesser beat-buesser changed the base branch from dev_1.8.0 to dev_1.7.2 August 11, 2021 11:07
@@ -76,7 +76,7 @@ def __init__(
num_trial: int = 25,
sample_size: int = 20,
init_size: int = 100,
min_epsilon: Optional[float] = None,
min_epsilon: float = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
min_epsilon: float = 0,
min_epsilon: Union[float, int] = 0,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PEP 484,

when an argument is annotated as having type float, an argument of type int is acceptable

so I think only float is sufficient as in epsilon and delta.

Copy link
Collaborator

@beat-buesser beat-buesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kaitokishi Thank you very much for your first pull request to ART! This is really great! I have created branch dev_1.7.2 now and updated the PR accordingly. I like removing the None, but I would like to suggest to apply typing and testing for int and float explicitly for good testing. What do you think?

ART 1.7.2 automation moved this from Pull request open to Pull request review Aug 11, 2021
Co-authored-by: Beat Buesser <49047826+beat-buesser@users.noreply.github.com>
@kaitokishi
Copy link
Contributor Author

kaitokishi commented Aug 16, 2021

Thank you for your suggestion. I think using Union[float, int] is redundent as I commented. Nevertheless, if you would like to write it explicitly, I accept your suggestion.

Copy link
Collaborator

@beat-buesser beat-buesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kaitokishi Thank you very much for you first pull request to ART!

@beat-buesser beat-buesser merged commit c6b3053 into Trusted-AI:dev_1.7.2 Aug 24, 2021
ART 1.7.2 automation moved this from Pull request review to Pull request done Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working improvement Improve implementation
Projects
No open projects
ART 1.7.2
  
Pull request done
Development

Successfully merging this pull request may close these issues.

Exception is raised even if min_epsilon is a positive float in BoundaryAttack
3 participants