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

Tests failing on travis due to missing scipy #18

Closed
eteq opened this issue Feb 3, 2015 · 1 comment · Fixed by #19
Closed

Tests failing on travis due to missing scipy #18

eteq opened this issue Feb 3, 2015 · 1 comment · Fixed by #19

Comments

@eteq
Copy link
Member

eteq commented Feb 3, 2015

The problem mentioned in #17 is now mostly fixed (a few of the builds still have it, but a fix is in the works for that from the astropy_helpers side). But now that the tests are actually running on travis, a different problem has showed up. To see an example, take a look at one of the failing builds from https://travis-ci.org/astropy/halotools/builds/49241697 . There, you'll see a bunch of errors like ImportError: No module named scipy.special.

What's happening there is that the default builds do not include scipy. This is because, in astropy, we have a general guideline that scipy is not a strict requirement, but rather it is necessary for certain things (like model fitting) that use scipy. So any test that invokes functionality requiring scipy is skipped if scipy is not installed, but run if it is. For an example of this, see https://github.com/astropy/astropy/blob/v1.0rc1/astropy/modeling/tests/test_fitters.py - you'll see a block at the top that defines the HAS_SCIPY variable, and then tests below that require scipy have decorators that look like @pytest.mark.skipif('not HAS_SCIPY').

As it stands, however, even that will not work for halotools - this is because there are at least some places where scipy is imported at the top level. (E.g., https://github.com/astropy/halotools/blob/master/halotools/halo_occupation.py imports a variety of things from scipy). In astropy, the convention (where possible) is to have scipy imports always inside a function/method, so that the error only happens when that functionality is used, rather than when the package is imported. So then stuff that requires scipy fails, but everything else can still succeed.

So there are two different solutions:

  1. Update halotools both to skip the scipy-requiring tests and move all scipy imports inside functions.
  2. Just take the stance that scipy is a dependency of halotools. setup.py should updated to add scipy to the requires list, and travis can be updated so that scipy is always installed (both of those are pretty much one-line changes).

I can set up option 2 myself if thats the way you want to go, @aphearin, but of course someone more familiar with halotools would have to do option 1.

@aphearin
Copy link
Contributor

aphearin commented Feb 3, 2015

Aha! Thanks for tracking this down, @eteq . I knew halotools was going to bump into this scipy requirement before the first official release, but I didn't anticipate this impacting the test suite, though your explanation makes sense. Scipy is so thoroughly integrated into halotools that I think it would be misleading to have halotools import without scipy. So option 2 is preferred: setup.py should be changed to install scipy upon halotools install.

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 a pull request may close this issue.

2 participants