Fix dtype-related issues of arange #3722
Conversation
mrocklin
left a comment
There was a problem hiding this comment.
This looks great! Thanks for tracking this down and fixing things @crusaderky
I've added a few comments, mostly cosmetic
dask/array/tests/test_creation.py
Outdated
| def test_arange_has_dtype(): | ||
| assert da.arange(5, chunks=2).dtype == np.arange(5).dtype | ||
| # Unexpected or missing kwargs | ||
| with assert_raises(TypeError): |
There was a problem hiding this comment.
In most of the codebase we use the fully qualified name with pytest.raises(...). I suggest that we stick to this for uniformity (and also fully qualified names are nice sometimes :)).
dask/array/tests/test_creation.py
Outdated
| assert da.arange(5, chunks=2).dtype == np.arange(5).dtype | ||
| # Unexpected or missing kwargs | ||
| with assert_raises(TypeError): | ||
| da.arange(10, chunks=-1, unexpected=True) |
There was a problem hiding this comment.
It would be nice (but not mandatory) to include the following check:
with pytest.raises(TypeError) as info:
da.arange(10, chunks=-1, unexpected=True)
assert 'unexpected' in str(info.value)| with assert_raises(TypeError): | ||
| da.arange(10, chunks=-1, unexpected=True) | ||
| with assert_raises(TypeError): | ||
| da.arange(10) |
There was a problem hiding this comment.
Same here, it might be nice (but not mandatory) to ensure that the user is pointed towards the chunks= keyword.
dask/array/tests/test_creation.py
Outdated
| def test_arange_dtypes(start, stop, step, dtype): | ||
| a_np = np.arange(start, stop, step, dtype=dtype) | ||
| a_da = da.arange(start, stop, step, dtype=dtype, chunks=-1) | ||
| assert a_np.dtype == a_da.dtype |
There was a problem hiding this comment.
I think that this should be checked automatically in assert_eq. If not then we should put it there instead.
There was a problem hiding this comment.
I know it is checked, however I don't know (and I don't want to rely upon) if it is checked before or after compute() which in this case makes a lot of difference
There was a problem hiding this comment.
I believe that it is checked both before and after. If I'm wrong and it isn't then it should be.
|
@mrocklin could you please assist with this CI failure? https://travis-ci.org/dask/dask/jobs/400540686#L1337-L1352 |
|
The CI failure has been patched up for now. I believe that you may have to merge with master though. |
|
@mrocklin new CI failure. Could you help again? |
|
Hrm, that's odd. Conda/pip failure? I wonder if there is a way to make
our CI system a bit more robust to package download failure.
…On Sat, Jul 7, 2018 at 1:54 PM, crusaderky ***@***.***> wrote:
@mrocklin <https://github.com/mrocklin> new CI failure. Could you help
again?
coverage run /home/travis/miniconda/envs/test-environment/bin/py.test dask --runslow --doctest-modules
coverage: command not found
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3722 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszBlZxLoDJDNgqmLNu6X7SnHmj-RAks5uEPXVgaJpZM4VDu0E>
.
|
|
Anyway, I've restarted for now
On Sat, Jul 7, 2018 at 2:07 PM, Matthew Rocklin <mrocklin@anaconda.com>
wrote:
… Hrm, that's odd. Conda/pip failure? I wonder if there is a way to make
our CI system a bit more robust to package download failure.
On Sat, Jul 7, 2018 at 1:54 PM, crusaderky ***@***.***>
wrote:
> @mrocklin <https://github.com/mrocklin> new CI failure. Could you help
> again?
>
> coverage run /home/travis/miniconda/envs/test-environment/bin/py.test dask --runslow --doctest-modules
> coverage: command not found
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#3722 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AASszBlZxLoDJDNgqmLNu6X7SnHmj-RAks5uEPXVgaJpZM4VDu0E>
> .
>
|
|
Looks good to me. Thanks @crusaderky ! |
Closes #3721
Fixes everything except complex numbers, which I don't know how they're supposed to work in plain numpy to begin with...