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

Inconsistent/Confusing behavior of util "safe_div" #654

Closed
aobo-y opened this issue Apr 19, 2021 · 2 comments
Closed

Inconsistent/Confusing behavior of util "safe_div" #654

aobo-y opened this issue Apr 19, 2021 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@aobo-y
Copy link
Contributor

aobo-y commented Apr 19, 2021

https://github.com/pytorch/captum/blob/master/captum/_utils/common.py#L26-L37

The behavior of how safe_div uses default_value is inconsistent depending on if denorm is float or tensor

Described in the the comments, seems the default_value should be used directly as the devision result when denorm is 0. This is indeed implemented accordingly when denorm is float

return (numerator / denom) if denom != 0.0 else default_value

However, when denorm is tensor, the default_value is actually used as the default denorm instead

return numerator / torch.where(denom != 0.0, denom, default_value)

I think the 2nd behavior is more reasonable, i.e., when denorm is float, update to

return numerator / (denom if denom != 0.0 else default_value)

Also better rename the parameter to default_denorm to avoid further confusion.
The question left is if there is any need or use cases for the 1st bahvior.

@NarineK
Copy link
Contributor

NarineK commented May 28, 2021

@aobo-y, that's a good point! I agree, let's change it to return numerator / (denom if denom != 0.0 else default_value).
default_denorm is a good name.
Thank you!

@NarineK NarineK added the bug Something isn't working label May 28, 2021
@aobo-y aobo-y self-assigned this May 30, 2021
facebook-github-bot pushed a commit that referenced this issue Sep 9, 2021
Summary:
According to the discussion #654
unify the util `safe_div`'s behavior with `default_denom`

add corresponding tests

Pull Request resolved: #751

Reviewed By: 99warriors

Differential Revision: D30829967

Pulled By: aobo-y

fbshipit-source-id: 11b1def02fef7570582431afec5f563c6559d47e
@aobo-y
Copy link
Contributor Author

aobo-y commented Sep 13, 2021

fixed in #751

@aobo-y aobo-y closed this as completed Sep 13, 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
Projects
None yet
Development

No branches or pull requests

2 participants