First attempt at BF for 2165 and added better tests for scalarmath pow function #349

Merged
merged 3 commits into from Apr 2, 2013

4 participants

@ericfode

This closes #621 but brings up what may be a new nastier bug.

It seems that the type conversions that you can build into ufuncs (for example in the codegen file for the npymath files) only work when you call the ufunc from python, and not when you call the generated function from c.

@travisbot

This pull request passes (merged a77a7cd into 143fb18).

@charris charris commented on the diff Jul 12, 2012
numpy/core/src/scalarmathmodule.c.src
@@ -494,16 +494,25 @@ half_ctype_remainder(npy_half a, npy_half b, npy_half *out) {
/**end repeat**/
/**begin repeat
- * #name = half, float, double, longdouble#
@charris
NumPy member
charris added a note Jul 12, 2012

Why remove the half? It is a 16 bit float.

Because it needed to be done by hand, because currently halfs are converted to floats and then worked with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/src/scalarmathmodule.c.src
*/
static npy_@name@ (*_basic_@name@_pow)(@type@ a, @type@ b);
+//called when ** is used (not performing properly)
@charris
NumPy member
charris added a note Jul 12, 2012

C++ comments aren't portable, use C style /.../

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/src/scalarmathmodule.c.src
static void
@name@_ctype_power(@type@ a, @type@ b, @type@ *out) {
- *out = _basic_@name@_pow(a, b);
+ *out = _basic_@name@_pow(a, b);
@charris
NumPy member
charris added a note Jul 12, 2012

Indentation is four spaces (not tabs, not two)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/src/scalarmathmodule.c.src
}
/**end repeat**/
+static void
+half_ctype_power(npy_half a,npy_half b, npy_half *out)
@charris
NumPy member
charris added a note Jul 12, 2012

OK, I see what you did.

@charris
NumPy member
charris added a note Jul 12, 2012

space after ',' in function argument list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/tests/test_scalarmath.py
@@ -58,7 +58,18 @@ def test_large_types(self):
assert_(b == 6765201, msg)
else:
assert_almost_equal(b, 6765201, err_msg=msg)
-
+ def test_mixed_types(self):
+ typelist = [np.int8,np.int16,np.float16,np.float32,np.float64,np.int8,np.int16,np.int32,np.int64]
@charris
NumPy member
charris added a note Jul 12, 2012

80 character limit on line length.

@charris
NumPy member
charris added a note Jul 12, 2012

You can also use the letter typecodes in np.typecodes. For instance

In [3]: for dt in typecodes['Float']: print dt
e
f
d
g
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/src/scalarmathmodule.c.src
*/
static npy_@name@ (*_basic_@name@_pow)(@type@ a, @type@ b);
+//called when ** is used (not performing properly)
static void
@name@_ctype_power(@type@ a, @type@ b, @type@ *out) {
@charris
NumPy member
charris added a note Jul 12, 2012

No yours, but '{' should be on next line.

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

Just made a quick scan for style.

The commit message needs expansion. The first line should be something like

BUG: ticket 2028, <short explanation>

<longer explanation> maybe a summary of the bug and what was done to fix it.
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/tests/test_scalarmath.py
@@ -58,7 +58,18 @@ def test_large_types(self):
assert_(b == 6765201, msg)
else:
assert_almost_equal(b, 6765201, err_msg=msg)
-
+ def test_mixed_types(self):
+ typelist = [np.int8,np.int16,np.float16,np.float32,np.float64,np.int8,np.int16,np.int32,np.int64]
+ for t1 in typelist:
+ for t2 in typelist:
+ a = t1(3)
+ b = t2(2)
+ o = a**b
+ msg = "error with %r and %r: got %r, expected %r" % (t1,t2,o,9)
@charris
NumPy member
charris added a note Jul 12, 2012

space between elements of the tuple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/tests/test_scalarmath.py
@@ -58,7 +58,18 @@ def test_large_types(self):
assert_(b == 6765201, msg)
else:
assert_almost_equal(b, 6765201, err_msg=msg)
-
+ def test_mixed_types(self):
+ typelist = [np.int8,np.int16,np.float16,np.float32,np.float64,np.int8,np.int16,np.int32,np.int64]
+ for t1 in typelist:
+ for t2 in typelist:
+ a = t1(3)
+ b = t2(2)
+ o = a**b
+ msg = "error with %r and %r: got %r, expected %r" % (t1,t2,o,9)
+ if np.issubdtype(np.dtype(o),np.integer):
@charris
NumPy member
charris added a note Jul 12, 2012

And spaces between function arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charris charris commented on an outdated diff Jul 12, 2012
numpy/core/tests/test_scalarmath.py
@@ -58,7 +58,18 @@ def test_large_types(self):
assert_(b == 6765201, msg)
else:
assert_almost_equal(b, 6765201, err_msg=msg)
-
+ def test_mixed_types(self):
+ typelist = [np.int8,np.int16,np.float16,np.float32,np.float64,np.int8,np.int16,np.int32,np.int64]
+ for t1 in typelist:
+ for t2 in typelist:
+ a = t1(3)
+ b = t2(2)
+ o = a**b
@charris
NumPy member
charris added a note Jul 12, 2012

'o'(utput?) is an unusual choice of variable name. Maybe something like tgt (target)?

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

This pull request passes (merged 6123625 into 143fb18).

@charris
NumPy member

Should be reanimated.

@ericfode

What needs to be done to get this merged?

@charris charris and 1 other commented on an outdated diff Apr 2, 2013
numpy/core/src/scalarmathmodule.c.src
@@ -970,7 +979,7 @@ static PyObject *
int retstatus;
int first;
@type@ out = {@zero@, @zero@};
-
+
@charris
NumPy member
charris added a note Apr 2, 2013

Trailing whitespace?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ericfode ericfode Update scalarmathmodule.c.src
Got rid of some white space in scalarmathmodule.c.src
8b42156
@charris
NumPy member

I can't figure out what issue this closes, the ones referenced don't look right.

@ericfode
@charris
NumPy member

OK, looks like it is #621 now.

@charris charris merged commit 134bcb0 into numpy:master Apr 2, 2013

1 check passed

Details default The Travis build passed
@charris
NumPy member

@certik Looks like this could be backported to 1.7.x. Eric, do you want to do that?

@ericfode
@certik

For reference, the backported PR is: #3187.

@ericfode ericfode referenced this pull request Apr 7, 2013
Merged

Float16pow #3187

@certik

I went over this PR and it looks good to me. Thanks!

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