fix undefined function and add integer divisions #3168

Merged
merged 1 commit into from Mar 28, 2013

Conversation

Projects
None yet
3 participants
@juliantaylor
Contributor

juliantaylor commented Mar 26, 2013

Fix a undefined function and some floats which appear through division and are used as slice indices.
Loosely based on the patch in https://bugs.launchpad.net/ubuntu/+source/python-numpy/+bug/701549

a = np.ndarray(shape=shape, dtype=type, buffer=buf)
if a.dtype.char == '?':
np.not_equal(a, 0, a)
return a
+
+# this function is referenced in the code above but not defined. adding
+# it back. - phensley

This comment has been minimized.

@njsmith

njsmith Mar 26, 2013

Member

This should be removed, because it won't be meaningful to anyone reading this code later -- it's a commit message, not a code comment.

@njsmith

njsmith Mar 26, 2013

Member

This should be removed, because it won't be meaningful to anyone reading this code later -- it's a commit message, not a code comment.

This comment has been minimized.

@juliantaylor

juliantaylor Mar 26, 2013

Contributor

just paranoia
I was pretty sure that its always a int, but better a nop than sorry

@juliantaylor

juliantaylor Mar 26, 2013

Contributor

just paranoia
I was pretty sure that its always a int, but better a nop than sorry

@@ -204,8 +204,8 @@ def fromfile(infile, type=None, shape=None, sizing=STRICT,
##file whose size may be determined before allocation, should be
##quick -- only one allocation will be needed.
- recsize = dtype.itemsize * np.product([i for i in shape if i != -1])
- blocksize = max(_BLOCKSIZE/recsize, 1)*recsize
+ recsize = int(dtype.itemsize * np.product([i for i in shape if i != -1]))

This comment has been minimized.

@njsmith

njsmith Mar 26, 2013

Member

Why is the call to int necessary here? How could the stuff on the right ever be anything other than an int?

@njsmith

njsmith Mar 26, 2013

Member

Why is the call to int necessary here? How could the stuff on the right ever be anything other than an int?

This comment has been minimized.

@juliantaylor

juliantaylor Mar 26, 2013

Contributor

just paranoia
I was pretty sure that its always a int, but better a nop than sorry
should I remove it?

@juliantaylor

juliantaylor Mar 26, 2013

Contributor

just paranoia
I was pretty sure that its always a int, but better a nop than sorry
should I remove it?

This comment has been minimized.

@njsmith

njsmith Mar 27, 2013

Member

Ah, okay. Yeah, it should be removed, because it's misleading to readers then. If you're still paranoid then the way to express that is

assert isinstance(recsize, int)
@njsmith

njsmith Mar 27, 2013

Member

Ah, okay. Yeah, it should be removed, because it's misleading to readers then. If you're still paranoid then the way to express that is

assert isinstance(recsize, int)
@njsmith

This comment has been minimized.

Show comment
Hide comment
@njsmith

njsmith Mar 27, 2013

Member

Oh also, should have thought of this before -- is there any easy way to write a test case that hits the undefined function? It looks like one way is to pass a file-like object that defines read() but not seek/tell (to get to line 230), and I can't see how to get to the call on line 252.

Member

njsmith commented Mar 27, 2013

Oh also, should have thought of this before -- is there any easy way to write a test case that hits the undefined function? It looks like one way is to pass a file-like object that defines read() but not seek/tell (to get to line 230), and I can't see how to get to the call on line 252.

@juliantaylor

This comment has been minimized.

Show comment
Hide comment
@juliantaylor

juliantaylor Mar 27, 2013

Contributor

is it worth adding a test for that? the function is simple enough
maybe one can use the already tested np.resize

time would probably be better spent replacing the whole function with a wrapper around np.fromfile instead of trying to test all the branches.

Contributor

juliantaylor commented Mar 27, 2013

is it worth adding a test for that? the function is simple enough
maybe one can use the already tested np.resize

time would probably be better spent replacing the whole function with a wrapper around np.fromfile instead of trying to test all the branches.

@njsmith

This comment has been minimized.

Show comment
Hide comment
@njsmith

njsmith Mar 27, 2013

Member

Oh man, I totally missed that this was the numarray compatibility code. Yeah, okay, that makes me less concerned about... well, everything...

This code is pretty much unmaintained and we're planning to drop it in a few releases in any case. Honestly I'd be fine with just merging this as is in that case, it won't reduce the overall quality of this module at all. Anyone object?

Member

njsmith commented Mar 27, 2013

Oh man, I totally missed that this was the numarray compatibility code. Yeah, okay, that makes me less concerned about... well, everything...

This code is pretty much unmaintained and we're planning to drop it in a few releases in any case. Honestly I'd be fine with just merging this as is in that case, it won't reduce the overall quality of this module at all. Anyone object?

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Mar 27, 2013

Member

+1

we shouldn't break it, but slightly lower standards for code quality and test coverage aren't a big issue imho

Member

rgommers commented Mar 27, 2013

+1

we shouldn't break it, but slightly lower standards for code quality and test coverage aren't a big issue imho

njsmith added a commit that referenced this pull request Mar 28, 2013

Merge pull request #3168 from juliantaylor/numarray-fixes
fix undefined function and add integer divisions

@njsmith njsmith merged commit db75eb4 into numpy:master Mar 28, 2013

1 check passed

default The Travis build passed
Details

@juliantaylor juliantaylor deleted the juliantaylor:numarray-fixes branch Nov 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment