Dec cleanup #18844
Dec cleanup #18844
Conversation
| import scipy | ||
| if (spmatrix is scipy.sparse.dok_matrix and LooseVersion( | ||
| scipy.__version__) >= LooseVersion('0.19.0')): | ||
| pytest.skip("dok_matrix from object does not work in SciPy >= 0.19") |
There was a problem hiding this comment.
This is a slightly different take on the existing code, which was the only place I saw the max_version argument being used. Rather than re-build that in the new module, I changed this around to be more explicit about the issue encountered when running this for SciPy v > 0.19.
Note that all of the scenarios in this test issue the warning that"object dtype is not supported by sparse matrices"
|
Looking into Travis failures. I was only grepping the pandas directory for |
|
|
||
| @pytest.fixture(params=['bsr', 'coo', 'csc', 'csr', 'dia', 'dok', 'lil']) | ||
| def spmatrix(request): | ||
| tm._skip_if_no_scipy() |
There was a problem hiding this comment.
Instead of calling it imperatively I replaced it with decorators in the test functions that were using this fixture
There was a problem hiding this comment.
Hmm...I think that we should still skip within the resource if it cannot be used. It is more decoupled that way IMO.
There was a problem hiding this comment.
So you'd rather remove the decorator and keep it to the fixture to skip tests? Or do both and live with the duplication? I'd argue either one violates TMTOWTDI given the former would make this inconsistent with other SciPy-requiring tests that don't need this fixture (which are the majority). Not to mention it would defeat the purpose of the original issue. The latter violation should be self-evident.
There was a problem hiding this comment.
Fair enough. You could just do scipy = pytest.importorskip(...), but if we're okay with leaving it to the developer to be mindful of that dependency, sure.
pandas/tests/io/test_pytables.py
Outdated
| pytest.raises(ValueError, store.put, 'b', df, | ||
| format='fixed', complib='zlib') | ||
|
|
||
| @td.skip_if_no('tables', min_version='2.2') |
There was a problem hiding this comment.
actually you can remove this, we require tables >= 3.0
There was a problem hiding this comment.
Removed altogether in last push - the module-level importorskip already prevents this from being run without pytables
|
thanks @WillAyd very nice set of patches! |
4 participants
git diff upstream/master -u -- "*.py" | flake8 --diffThis should be the last commit to close out the issue referenced above. I think there's further opportunity to convert some of the imperative
pytest.skipcalls over to the new decorator methodology (especially Ibnpandas/tests/io) but to keep the scope clean I would rather open a new issue for those rather than continually update #18190