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

Avoid adjusting gufunc's meta dtype twice #5274

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@pentschev
Copy link
Member

commented Aug 14, 2019

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

This fixes the issue in #5270. A brief explanation to what was causing this, is that we had already computed the meta's dtype in https://github.com/dask/dask/blob/master/dask/array/gufunc.py#L426-L446. While we have to adjust the final shape in https://github.com/dask/dask/blob/master/dask/array/gufunc.py#L481, the dtype has been already previously computed correctly.

The test here will pass either way, because the failing test mentioned in #5270 is marked to xfail. I don't really know what the comment https://github.com/dask/dask/blob/master/dask/array/tests/test_array_core.py#L257 means. Does anybody knows if it's safe to remove the xfail and just keep the skipif NumPy < 1.14?

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Thanks @pentschev , can we remove the xfail in test_Array_numpy_gufunc_call__array_ufunc__02 as was done in the original issue in order to test this?

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

That was my question @mrocklin, the original issue #5220 only sets strict=False https://github.com/dask/dask/pull/5220/files#diff-08cca2be1d0ddcb396862ead9f5b3089R259-R261, together with a project-wide xfail_strict=True https://github.com/dask/dask/pull/5220/files#diff-380c6a8ebbbce17d55d50ef17d3cf906R46.

Are you suggesting I remove the xfail for test_Array_numpy_gufunc_call__array_ufunc__02, or are you suggesting that I make it not strict, by adding both changes I mentioned above?

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Ah, sorry, I was rushing through things. Well, I suggest that we remove the xfail and see if things work. If so, great!

@pentschev

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Done, let's see what happens.

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Tests pass. Thanks @pentschev . Let's merge this in and move on :)

@mrocklin mrocklin merged commit 281fd90 into dask:master Aug 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jrbourbeau

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Sorry, just seeing this now. LGTM, thanks for the fix @pentschev!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.