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

MRG: compatibility with numpy 1.12.0 #419

Merged
merged 27 commits into from
Jan 26, 2017

Conversation

matthew-brett
Copy link
Member

Numpy 1.12 started raising errors for float indices in a variety of
circumstances, causing lots of test errors - e.g.
https://travis-ci.org/nipy/nipy/jobs/193952659

Numpy 1.12 more strict about enforcing integers for indices.
Fix errors for Numpy 1.12 and floating point values used as indices.
Make indices integers; make boolean index match size of indexed array.
Make general function for multiplying a sequence of numbers. Use to
return integer from product of array shape.
Enforce integer indices in kernel smooth indexing.
Make sure indices are integers rather than floats or arrays.
Force shape elements to ints for indexing.
Indexing now requires integers; force indices to ints, replicating
previous behavior.
math.floor returns an integer.
The row selection for these tests was broken, depending on a single
False for an empty array.  The rows were being selected by comparison to
an int, but the code changed to produce character strings some time
back, so no rows were being found.
Add doctest flag to test equality of arrays reqpresented by their reprs
in docstrings.  This also fixes a change in the display of integers as
floats. It appears that numpy 1.12 displays "1." compared to previous
numpy display of "1.0".
Make _slicer a method rather than a lambda, and expand names for
clarity.

On the way, ensure slice indices are ints, not floats.
Make new slice copies at each iteration to prevent later iterations
overwriting the earlier slices in memory.

Extend tests.
A backport from a fix in nibabel.
math.ceil, math.floor, round generate integers in Python 3 but not
Python 2.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 471da90 on matthew-brett:np-12-compat into ** on nipy:master**.

To allow for unresolved mroi test failure -
nipy#420.
@codecov-io
Copy link

codecov-io commented Jan 24, 2017

Current coverage is 83.18% (diff: 89.06%)

No coverage report found for master at 438d51f.

Powered by Codecov. Last update 438d51f...b148483

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a94a998 on matthew-brett:np-12-compat into ** on nipy:master**.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. A couple questions about division that I may just not have enough context for.

@@ -234,7 +236,7 @@ def cross_validated_update(self, x, z, plike, kfold=10):
if np.isscalar(kfold):
aux = np.argsort(np.random.rand(n_samples))
idx = - np.ones(n_samples).astype(np.int)
j = np.ceil(n_samples / kfold)
j = int(math.ceil(n_samples / kfold))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't n_samples / kfold already be an integer (at least in Python 2)? Or does from __future__ import division not need to happen in every file where it's used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will be an integer in Python 2. I guess the question for @bthirion is - was this intended to be true (floating point division) even in Python 2? At the moment this can give different answers on Python 2 and Python 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is an int. I think that the result is equivalent in Python 2 and 3 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kfold doesn't divide evenly, it will be off by one:

Suppose n_samples = 5, kfold = 2.

Python 2: int(math.ceil(5 / 2)) == int(math.ceil(2)) == int(2.0) == 2
Python 3: int(math.ceil(5 / 2)) == int(math.ceil(2.5)) = int(3.0) == 3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. In that case, we don't really care about the exact value, so both are fine.

@@ -157,7 +159,7 @@ def ward_test_more(n=100, k=5, verbose=0):
# Check that two implementations give the same result
np.random.seed(0)
X = randn(n,2)
X[:np.ceil(n/3)] += 5
X[:int(math.ceil(n / 3))] += 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same integer issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bthirion - how about this one? Presumably meant to be true division?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it is n // 3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bthirion - I was thinking you meant this be floating point division, otherwise the ceil call doesn't make sense, because the integer division does an implicit floor on this positive number. But you did mean integer division? In which case we can discard the ceil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Use true division / math.ceil to find indices.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b148483 on matthew-brett:np-12-compat into ** on nipy:master**.

@matthew-brett matthew-brett merged commit e93e1aa into nipy:master Jan 26, 2017
@matthew-brett
Copy link
Member Author

Thanks for the review and comments.

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.

None yet

5 participants