Deprecate and rename conflicting argument names #558
Conversation
There was a problem hiding this comment.
Looks great, thanks for updating this :) ! I think we might also need to update the generated tests here https://github.com/pytorch/captum/blob/master/tests/attr/helpers/test_config.py with the updated names, but can also leave it for now and update when we deprecate to continue testing the older names, either way works.
captum/attr/_utils/common.py
Outdated
|
|
||
| @functools.wraps(func) | ||
| def wrapper(*args: Any, **kwargs: Any): | ||
| if "n_samples" in kwargs: |
There was a problem hiding this comment.
I think this condition can also check that nt_samples is not already in kwargs? Otherwise, it wouldn't be possible to pass both for Lime and KernelShap until this is deprecated right?
There was a problem hiding this comment.
yeah, that's a good point!
There was a problem hiding this comment.
@NarineK has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@NarineK has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@NarineK has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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
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:
n_samplesin NoiseTunnel is being renamed tont_samplesn_perturbed_samplesin Lime and KernelSHAP are being renamed ton_samplesin order to remain consistent with Shapely Values (Sampling)