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

Improve compatibility with 32bit architectures #2937

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

mrocklin
Copy link
Member

@mrocklin mrocklin commented Nov 30, 2017

This replaces np.foo64 with np.foo_ and vice versa for foo in {float, int}

See #20

  • Tests added / passed
  • Passes flake8 dask
  • Fully documented, including docs/source/changelog.rst for all changes
    and one of the docs/source/*-api.rst files for new API

cc @QuLogic

@pitrou
Copy link
Member

pitrou commented Nov 30, 2017

Two things:

  • I wouldn't change np.float64 (it doesn't have anything with the architecture being 32- or 64-bit)
  • I would use np.intp for integers

See https://docs.scipy.org/doc/numpy-1.13.0/user/basics.types.html#array-types-and-conversions-between-types for reference

This replaces np.int64 with np.intp in a few cases

See dask#20
@mrocklin
Copy link
Member Author

mrocklin commented Nov 30, 2017 via email

@pitrou
Copy link
Member

pitrou commented Nov 30, 2017

LGTM. @jakirkham, any concerns?

@jakirkham
Copy link
Member

Not really. Looks pretty clean to me. 👍 Happy to see this was relatively straightforward.

Should we be adding 32-bit to Travis or another CI to avoid regressions?

Side note: Guessing the xref in the OP was suppose to be issue ( #2507 ) (unless I'm missing something).

@mrocklin
Copy link
Member Author

Side note: Guessing the xref in the OP was suppose to be issue ( #2507 ) (unless I'm missing something).

Yup. Thanks.

@mrocklin
Copy link
Member Author

I noticed some Windows errors in Appveyor, so this might not be complete yet.

@pitrou
Copy link
Member

pitrou commented Nov 30, 2017

I noticed some Windows errors in Appveyor, so this might not be complete yet.

Probably because np.int_ is a 32-bit int on 64-bit Windows (Windows 64 follows the LLP64 model).

@jakirkham
Copy link
Member

Probably true. It would be nice if Windows was less weird with integers. 😞

@mrocklin
Copy link
Member Author

Tests pass. Merging this soon if there are no further concerns.

@jakirkham jakirkham merged commit da2cb1f into dask:master Nov 30, 2017
@jakirkham
Copy link
Member

Thanks @mrocklin.

@mrocklin mrocklin deleted the 32bit-compat branch November 30, 2017 19:24
@QuLogic
Copy link
Contributor

QuLogic commented Dec 5, 2017

OK, this fixes all the new failures from 0.16.0, but not the repartition ones originally from #2507.

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 this pull request may close these issues.

4 participants