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

Add scipy as a dependency to setup.py #675

Merged
merged 2 commits into from
Dec 6, 2018
Merged

Add scipy as a dependency to setup.py #675

merged 2 commits into from
Dec 6, 2018

Conversation

willfrey
Copy link
Contributor

@willfrey willfrey commented Dec 4, 2018

scipy is imported in both the torchvision.datasets.svhn module and the torchvision.models.inception module.

Alternatively, we could put this under extras_require and make the user explicitly request to install scipy as an additional dependency.

`scipy` is imported in both the `torchvision.datasets.svhn` module and the `torchvision.models.inception` module.
@fmassa
Copy link
Member

fmassa commented Dec 4, 2018

scipy is an optiional dependency of torchvision, and is not required in most cases (that's why it's imported inside the function.

Is there a way to make it optional in the installation process?

Make scipy optional. It can be installed with `pip install torchvision[scipy]`.
@willfrey
Copy link
Contributor Author

willfrey commented Dec 4, 2018

I've made the change to make it optional.

Just my two cents but I'd argue that importing it within a function and not having a graceful/seamless experience for the client means that it's very much a dependency. Having to just "know" that you need scipy for two particular methods isn't the nicest!

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit 5123ded into pytorch:master Dec 6, 2018
@fmassa
Copy link
Member

fmassa commented Dec 6, 2018

So, this is kind of debatable.

I've myself never used any of the modules that require scipy.
Also, PyTorch up until very recently didn't have a dependency on numpy either, it was optional.

But thanks for the PR!

@willfrey
Copy link
Contributor Author

willfrey commented Dec 6, 2018

In my opinion, it's nice to have the dependency explicitly written at least somewhere in the setup.py file, where folks are most likely to look first.

Thanks for merging!

@willfrey willfrey deleted the patch-2 branch December 6, 2018 14:00
rajveerb pushed a commit to rajveerb/vision that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants