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

Updating import instructions for scipy, sklearn - issue #848 #891

Merged
merged 3 commits into from Mar 10, 2018

Conversation

Projects
None yet
5 participants
@jessica-mitchell
Contributor

jessica-mitchell commented Feb 13, 2018

This PR is addressing the issue #848, which notes that you have import sklearn before nest, as well as scipy. This is not a fix but just a documentation change to make people aware of it. I added a comment to the PyNEST tutorial regarding the import order of both scipy and sklearn. Additionally, I added a line to indicate the import order (wrt scipy) in the CampbellSiegert example, since it was the only example that used scipy that did not explicitly mention the order needed.
There are also a couple of minor changes to the PyNEST tutorial, unrelated to the import issue but brought to my attention as mistakes in the docs that I hope would be ok to add into this PR.

@jougs jougs requested review from jougs and Silmathoron Feb 19, 2018

@Silmathoron

I would like to make sure that the scipy bug really still exists... based on what @jougs said during the VC, it should not (and does not on my computer).

Regarding sklearn, they use lazy import so we would need to import all the submodules to fix it. While not hard, it is not elegant and I don't now if it is reasonnable. I am currently investigating the segfault to see whether a nice solution exists.

@@ -34,6 +34,11 @@
Sven Schrader, Nov 2008, Siegert implementation by Tom Tetzlaff
'''
'''
First, we import all necessary modules for simulation and analysis. Scipy

This comment has been minimized.

@Silmathoron

Silmathoron Feb 19, 2018

Contributor

On my computer, putting the two scipy imports after nest does not lead to any kind of trouble, can you post what the error message you get is?
(python 3.6.4, scipy 0.19)

@Silmathoron

Silmathoron Feb 19, 2018

Contributor

On my computer, putting the two scipy imports after nest does not lead to any kind of trouble, can you post what the error message you get is?
(python 3.6.4, scipy 0.19)

This comment has been minimized.

@jougs

jougs Mar 6, 2018

Contributor

The original issue we experienced was numpy/numpy#2521, apparently fixed in numpy/numpy@a226daa.

As the suggested import order does not matter in most cases and prevents a crash in the best case, I like the proposed addition although most people will not be hit by the long-fixed issue anymore.

@jougs

jougs Mar 6, 2018

Contributor

The original issue we experienced was numpy/numpy#2521, apparently fixed in numpy/numpy@a226daa.

As the suggested import order does not matter in most cases and prevents a crash in the best case, I like the proposed addition although most people will not be hit by the long-fixed issue anymore.

It should be noted, however, that certain external packages must be imported *before* importing nest.
These include `sklearn` ([scikit-learn](http://scikit-learn.org/stable/index.html)) and `scipy` ([SciPy](https://www.scipy.org/)):
from sklearn.svm import LinearSVC

This comment has been minimized.

@Silmathoron

Silmathoron Feb 19, 2018

Contributor

I can confirm the sklearn bug on Python 3.6.4 and SKL 0.19.1, but not with scipy.
Also importing only sklearn and not the specific submodule does not solve the problem.

@Silmathoron

Silmathoron Feb 19, 2018

Contributor

I can confirm the sklearn bug on Python 3.6.4 and SKL 0.19.1, but not with scipy.
Also importing only sklearn and not the specific submodule does not solve the problem.

This comment has been minimized.

@jougs

jougs Mar 6, 2018

Contributor

@Silmathoron thanks for double-checking. In this case I agree with the suggested changes.

@jougs

jougs Mar 6, 2018

Contributor

@Silmathoron thanks for double-checking. In this case I agree with the suggested changes.

Show outdated Hide outdated ...as/userdoc/md/documentation/part-1-neurons-and-simple-neural-networks.md
@@ -34,6 +34,11 @@
Sven Schrader, Nov 2008, Siegert implementation by Tom Tetzlaff
'''
'''
First, we import all necessary modules for simulation and analysis. Scipy

This comment has been minimized.

@jougs

jougs Mar 6, 2018

Contributor

The original issue we experienced was numpy/numpy#2521, apparently fixed in numpy/numpy@a226daa.

As the suggested import order does not matter in most cases and prevents a crash in the best case, I like the proposed addition although most people will not be hit by the long-fixed issue anymore.

@jougs

jougs Mar 6, 2018

Contributor

The original issue we experienced was numpy/numpy#2521, apparently fixed in numpy/numpy@a226daa.

As the suggested import order does not matter in most cases and prevents a crash in the best case, I like the proposed addition although most people will not be hit by the long-fixed issue anymore.

Show outdated Hide outdated ...as/userdoc/md/documentation/part-1-neurons-and-simple-neural-networks.md
It should be noted, however, that certain external packages must be imported *before* importing nest.
These include `sklearn` ([scikit-learn](http://scikit-learn.org/stable/index.html)) and `scipy` ([SciPy](https://www.scipy.org/)):
from sklearn.svm import LinearSVC

This comment has been minimized.

@jougs

jougs Mar 6, 2018

Contributor

@Silmathoron thanks for double-checking. In this case I agree with the suggested changes.

@jougs

jougs Mar 6, 2018

Contributor

@Silmathoron thanks for double-checking. In this case I agree with the suggested changes.

Jessica Mitchell and others added some commits Mar 8, 2018

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 9, 2018

Contributor

@jougs @Silmathoron Are you happy with the PR now?

Contributor

heplesser commented Mar 9, 2018

@jougs @Silmathoron Are you happy with the PR now?

@Silmathoron

All good 👍

@jougs

jougs approved these changes Mar 9, 2018

@heplesser heplesser merged commit 166b9ea into nest:master Mar 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment