Repository navigation
Conversation
|
@ibusko - |
|
Thanks, @ibusko. Large data could go in https://github.com/astropy/astropy-data or https://github.com/astropy/photutils-datasets |
|
Moved larger data sets to photutils-datasets; modified tests so they run with the new paths. |
| from __future__ import (absolute_import, division, print_function, unicode_literals) | ||
|
|
||
| import numpy as np | ||
| from scipy.optimize import leastsq |
There was a problem hiding this comment.
@ibusko - scipy is currently an optional dependency of photutils, so this will fail the tests.
In other modules (e.g segmentation) this is avoided by importing scipy in the function/class rather on modul level. Corresponding tests then are skipped when no scipy install is available.
There was a problem hiding this comment.
My initial understanding (was wrong) is that this is a requirement of the tests only. Now I notice that the code itself can't rely on scipy. But the scipy.optimize.leastsq function is at the very backbone of the entire isophote fitting idea. In a case like this, we cannot have scipy as an optional dependency; it's mandatory. Does it mean that the isophote code can't live in photutils after all?
There was a problem hiding this comment.
@ibusko A lot of the functionality in photutils requires scipy and scikit-image, but they are "optional" dependencies. In other words, import photutils will still work without those dependencies. However, the actual functions that need those packages will raise an ImportError if they are not installed. This mirrors the behavior in astropy, where numpy is the only "strict" dependency.
@bsipocz Perhaps we should just make scipy and scikit-image required dependencies? I don't recall if affiliated packages are not supposed to have required dependencies other than numpy and astropy.
| import math | ||
|
|
||
| import numpy as np | ||
| import unittest |
There was a problem hiding this comment.
unittest is currently failing with python3.6 as we still use the astropy shipped pytest version.
@larrybradley - I already have an open PR to use system pytest, that should fix this (after an minor update), but I wonder whether we aim to have a pure pytest testing suite or mixing it with unittest is also OK?
There was a problem hiding this comment.
I was wondering about that too. I can convert the unit tests to pytest if that's what we want.
There was a problem hiding this comment.
It would be my preference to use only pytest.
There was a problem hiding this comment.
OK, I tend to lean toward py.test too, so the package better conforms to the standards used elsewhere. I guess I was just waiting for an excuse to do the change :-)
I will work on that and report here when done.
|
Modified all tests to conform to pytest style. It seems that all tests that depend on test data are failing because they can't find the data. Are there any examples in the photutils tests that I can use as a template for finding data files? |
| sample = Sample(test_data, 40.) | ||
| sample.update() | ||
|
|
||
| assert sample.mean == pytest.approx(200.02, abs=0.01) |
There was a problem hiding this comment.
These are all failing at the moment, as we are still using the astropy bundled ancient version of pytest.
@larrybradley - what do you think, shall we aim to merge #491 asap or rather work around to still support pytest 2.x?
|
|
||
| import sys | ||
| import numpy as np | ||
| from scipy.interpolate import LSQUnivariateSpline |
There was a problem hiding this comment.
in addition of moving these imports, the tests that use any code relying on scipy also need to be decorated with @pytest.mark.skipif('not HAS_SCIPY') where HAS_SCIPY is defined as
try:
import scipy
HAS_SCIPY = True
except ImportError:
HAS_SCIPY = False
e.g.: photutils/segmentation/tests/test_core.py
|
What is the state of this pull request? There are errors caused by failures in locating the data sets required by the tests. I understand that these data sets should live in photutils-datasets, but how exactly the packages are layered during the tests so the code can actually run and find the data files it needs? |
|
Did anyone get back to Ivo on this? @larrybradley, @bsipocz |
|
I plan to look over this PR this week. |
|
I kind of want to start pointing people to a Python version when I get |
|
I definitely would like to to so as well. As far as I know, this is ready to be merged. Although there still be issues with the location of large files, both for the test images, and for the images used in the tutorials. |
|
Sorry for the delay, but I plan to get to this soon. I would like to include this in the next release. |
|
@larrybradley any updates on this? |
5 participants
Tests run. Not sure that the documentation builds though (I get a "no such file" sphinx error).
The tutorial notebooks where moved to astropy-tutorials.
There are a few largish test data files that I don't know where to move to.