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.

Sign up for GitHub

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

Jump to bottom

n_perturb_samples on captum.metrics.sensitivity_max #549

Closed
gkakogeorgiou opened this issue Dec 4, 2020 · 4 comments
Closed

n_perturb_samples on captum.metrics.sensitivity_max #549

gkakogeorgiou opened this issue Dec 4, 2020 · 4 comments

Comments

@gkakogeorgiou
Copy link

The n_perturb_samples variable is also used in Lime definition. This means that you can not use n_perturb_samples to **kwargs on metrics.sensitivity_max, in case you want to use Sensitivity algorithm on Lime for specific n_perturb_samples. I propose to change the name of n_perturb_samples on sensitivity_max algorithm.
@NarineK
Copy link
Contributor

NarineK commented Dec 4, 2020 •
edited
Loading

Thank you for bringing this up, @gkakogeorgiou !
n_perturb_samples is used in both in sensitivity_max and infidelity metrics.
For Lime and KernelSHAP we can probably rename that variable to n_surrogate_samples ?
What do you think, @vivekmig ?

@vivekmig
Copy link
Contributor

vivekmig commented Dec 7, 2020

Yeah, this is a good point! We should avoid argument name overlaps with metrics. Could we possibly just use n_samples in Lime and KernelSHAP, which would match other attribution methods?

@NarineK
Copy link
Contributor

NarineK commented Dec 8, 2020

n_samples sounds good. One thing only - n_samples is also used in noise tunnel. It is less likely that someone will use Lime and KernelSHAP w/noise tunnel but just in case ?

@NarineK NarineK mentioned this issue Dec 11, 2020
Closed
facebook-github-bot pushed a commit that referenced this issue Dec 15, 2020
@NarineK @facebook-github-bot
Deprecate and rename conflicting argument names (#558)
ed558b0 
Summary:
As also mentioned in #549, we had conflicting argument names in the API. In this PR we deprecate and rename those argument names. More specifically:
1. `n_samples` in NoiseTunnel is being renamed to `nt_samples`
2. `n_perturbed_samples` in Lime and KernelSHAP are being renamed to `n_samples` in order to remain consistent with Shapely Values (Sampling)

Pull Request resolved: #558

Reviewed By: edward-io

Differential Revision: D25514136

Pulled By: NarineK

fbshipit-source-id: 142c974da2a8430be234fe4ffc79e36faf2bf8d9
@NarineK
Copy link
Contributor

NarineK commented Dec 16, 2020 •
edited
Loading

This got resolved with #558 , @gkakogeorgiou .
We will create a patch release to reflect these changes. Thank you for bringing it up.

@NarineK NarineK closed this as completed Dec 28, 2020
vivekmig pushed a commit that referenced this issue Jan 21, 2021
@NarineK @vivekmig
Deprecate and rename conflicting argument names (#558)
3d9a649 
Summary:
As also mentioned in #549, we had conflicting argument names in the API. In this PR we deprecate and rename those argument names. More specifically:
1. `n_samples` in NoiseTunnel is being renamed to `nt_samples`
2. `n_perturbed_samples` in Lime and KernelSHAP are being renamed to `n_samples` in order to remain consistent with Shapely Values (Sampling)

Pull Request resolved: #558

Reviewed By: edward-io

Differential Revision: D25514136

Pulled By: NarineK

fbshipit-source-id: 142c974da2a8430be234fe4ffc79e36faf2bf8d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vivekmig @NarineK @gkakogeorgiou