Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Reduce unnecessary reliance and usage of numpy #746

Closed
aobo-y opened this issue Aug 31, 2021 · 4 comments
Closed

Reduce unnecessary reliance and usage of numpy #746

aobo-y opened this issue Aug 31, 2021 · 4 comments
Assignees

Comments

@aobo-y
Copy link
Contributor

aobo-y commented Aug 31, 2021

This search gives us all the usage of Numpy in Captum https://github.com/pytorch/captum/search?p=1&q=numpy
Many of them are not necessary

  • either empty imports
  • or similar functionality can be replaced with pytorch

In order to reduce the unneeded dependency, we can investigate the current usages and remove/replace them when possible

@aobo-y aobo-y self-assigned this Aug 31, 2021
@bilalsal
Copy link
Contributor

Excellent find, @aobo-y!
Indeed, we noticed significant performance gain when we avoided numpy() while working on CCA and CKA.
You might need to pay attention to numerical precision issues, esp. to ensure that the unit tests continue to work meaningfully.

Thank you for bringing up this good proposal!

@aobo-y
Copy link
Contributor Author

aobo-y commented Sep 1, 2021

Thank you for the heads up @bilalsal !
I originally noticed this issue when helping @NarineK remove numpy for an internal use case (#714)
I will create a series of similar small PRs each of which contains only tightly-related changes, instead of one PR for all, so that we can review and discuss the removal/replacement case by case.

facebook-github-bot pushed a commit that referenced this issue Sep 10, 2021
Summary:
as title, for issue #746

Pull Request resolved: #755

Reviewed By: bilalsal

Differential Revision: D30857228

Pulled By: aobo-y

fbshipit-source-id: fbf258bc0af2ab1d39557678fd31ccbe37594669
@NarineK
Copy link
Contributor

NarineK commented Sep 23, 2021

@aobo-y can we close this issue since it has been addressed ?

@aobo-y
Copy link
Contributor Author

aobo-y commented Sep 23, 2021

@NarineK sure, I think we almost cleared the unnecessary cases.

However, there are still some tricky ones left. For example,

rand_coefficient = torch.tensor(
np.random.uniform(0.0, 1.0, inputs[0].shape[0]),
device=inputs[0].device,
dtype=inputs[0].dtype,
)

is perfect scenario to directly apply torch.rand(...). But the change will require quite some extra efforts to fix many tests, because even with fixed seed during tests, torch's random gives different values from np. As you can imagine, it needs update of the expected values and relaxation in many "almost-equal" asserts.
I will create separate issues dedicated for them.

@aobo-y aobo-y closed this as completed Sep 23, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants