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.

Sign up for GitHub

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

Jump to bottom

fix int type usage #1391

Closed
wants to merge 2 commits into from
Closed

fix int type usage #1391

wants to merge 2 commits into from

Conversation

rathann
Copy link
Contributor

@rathann rathann commented Jun 9, 2017

Fixes part of #1362 .

Changes made in this Pull Request:

  • int type usage fixes to accommodate 32 bit architectures
  • slight precision relaxation in analysis/test_psa.py:_BaseHausdorffDistance.test_symmetry comparison

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

rathann added 2 commits June 9, 2017 11:45
@rathann
use correct int types
51649ad 
This fixes ERRORs and FAILs in the testsuite on 32bit:

TypeError: Cannot cast array data from dtype('int64') to dtype('int32') according to the rule 'safe'

and

assert_(out[0].dtype == np.int64)
  File "/usr/lib/python2.7/site-packages/numpy/testing/utils.py", line 92, in assert_
    raise AssertionError(smsg)
AssertionError
@rathann
relax test_symmetry test due to lower precision on 32bit
280dc13
@rathann
Copy link
Contributor Author

rathann commented Jun 9, 2017

I'm also wondering if MDAnalysis/core/groups.py:2739 should be changed to np.intp as well.

@kain88-de
Copy link
Member

@rathann do you know of a way of testing if this is robust to run on 32bit using a 64bit travis machine?

@orbeckst
Copy link
Member

Maybe follow the recipe in travis-ci/travis-ci#5770 and use a docker image with a 32 bit chroot for testing on travis?

@orbeckst
Copy link
Member

orbeckst commented Jun 10, 2017 •
edited
Loading

And obspy/obspy#1580 was apparently a successful attempt to Enable 32bit linux on travis CI without docker, just using compat 32 bit libs and the 32-bit miniconda. And they use Python 2.7 and 3.5.

@orbeckst
Copy link
Member

According to numpy/numpy#4384 (comment), np.intp is the recommended dtype for indexing

... use np.intp as dtype whenever things have to do with indexing or are logically related to indexing/array sizes. This is the natural dtype for it and it will normally also be the fastest one.

This seems to imply that we ought to be using it everywhere instead of np.int64 — I don't know if @rathann 's PR here is already replacing all occurrences.

@rathann
Copy link
Contributor Author

rathann commented Jun 11, 2017

@kain88-de

@rathann do you know of a way of testing if this is robust to run on 32bit using a 64bit travis machine?

I don't know travis, but on Fedora we use 32bit chroot or container to do 32bit builds on a 64bit machine via mock: mock -r fedora-rawhide-i386.

@rathann
Copy link
Contributor Author

rathann commented Jun 11, 2017

@orbeckst

This seems to imply that we ought to be using it everywhere instead of np.int64 — I don't know if @rathann 's PR here is already replacing all occurrences.

Not everywhere, only for array indexing. And no, I'm not replacing all. Just those that made the tests fail. I'm not familiar with the codebase.

@jbarnoud jbarnoud mentioned this pull request Jun 11, 2017
Closed
4 tasks
@richardjgowers richardjgowers mentioned this pull request Jun 12, 2017
Merged
5 tasks
@richardjgowers
Copy link
Member

I've made a new PR which extends this a little with some tests. I've changed all the arrays we use for indexing to use np.intp. We don't need to change all integer array dtypes to intp as some integer arrays are data, eg resids, rather than indices.

orbeckst added a commit that referenced this pull request Jun 14, 2017
@orbeckst
Merge pull request #1393 from MDAnalysis/issue-1362-int_types
6e6aa9d 
Issue #1362 : use np.intp types for indexing instead of np.int64

- According to numpy/numpy#4384 (comment), `np.intp` is the recommended dtype for indexing:
   "... use np.intp as dtype whenever things have to do with indexing or are logically related to 
   indexing/array sizes. This is the natural dtype for it and it will normally also be the fastest one.
- see https://docs.scipy.org/doc/numpy/user/basics.types.html
- see also PR #1391 for discussion
- This change should go some way towards increasing compatibility with i386/i696 (32 bit) platforms.

NOTE: No dedicated testing on 32 bit yet (see WIP PR #1392 )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
32 bit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rathann @kain88-de @orbeckst @richardjgowers