ENH: Implement ndarray.__format__ for 0d arrays #9883
Conversation
This fixes numpygh-7978 The behavior for other sized arrays is left unchanged, pending discussion in numpygh-5543
numpy/core/src/multiarray/strfuncs.c
Outdated
| if (PyArray_NDIM(self) == 0) { | ||
| PyObject *item = PyArray_ToScalar(PyArray_DATA(self), self); | ||
| PyObject *res; | ||
| if (item == NULL) { |
There was a problem hiding this comment.
Above this line, I assume you mean?
|
|
||
| def test_0d(self): | ||
| a = np.array(np.pi) | ||
| assert_equal('{:0.3g}'.format(a), '3.14') |
There was a problem hiding this comment.
Is assert_equal the best way to compare strings? Maybe just assert_(str1 == str2) would be a bit cleaner.
There was a problem hiding this comment.
Yes, because then it shows the two values if they differ.
There was a problem hiding this comment.
In pytest you can just assert str1 == str2 and you will get values if there is a failure.
def test_failure():
a = 'apple'
b = 'banana'
assert a == b
Output is
================================== FAILURES ===================================
________________________________ test_failure _________________________________
def test_failure():
a = 'apple'
b = 'banana'
> assert a == b
E AssertionError: assert 'apple' == 'banana'
E - apple
E + banana
test_util.py:20: AssertionError
========================== 1 failed in 0.24 seconds ===========================
There was a problem hiding this comment.
For consistency, we should probably use the same form we use in most of the test suite, which gives:
>>> np.testing.assert_equal('a', 'b')
AssertionError:
Items are not equal:
ACTUAL: 'a'
DESIRED: 'b'
AFAIK, the switch to pytest was recent, and we were previously using nose
There was a problem hiding this comment.
We are still using nose, the pytest conversion is still in progress and I don't think I will get back to it until 1.15.
| Py_DECREF(item); | ||
| return res; | ||
| } | ||
| /* Everything else - use the builtin */ |
There was a problem hiding this comment.
It keeps the arbitrary and python-dependent behavior that we had before this patch.
Most importantly it handles '{}'.format in the typical way. On python 3, it throws an exception if a format string is given, while on python 2 it evaluates format(str(arr), 'theformatspec').
|
Seems reasonable, just a couple of nits and some questions. |
|
Added the blank line. If @ahaldane signs off I'll merge this (or he can). |
|
I will push the button. Thanks for a nice fix @eric-wieser! |
| # Could switch on python version here, but all we care about is | ||
| # that the behaviour hasn't changed | ||
| assert_equal( | ||
| ret_and_exc(object.__format__, a, '30'), |
There was a problem hiding this comment.
I suppose we should silence warnings in ret_and_exc, or return a third tuple item which is the warning
There was a problem hiding this comment.
Maybe just figure out what the result should be and use that? Eventually object.__format__ is going to fail with non-empty argument.
There was a problem hiding this comment.
Right, and I think that ndarray.__format__ should start failing at the same time. Maybe we should just make ndarray.__format__ fail with an empty argument right now...
There was a problem hiding this comment.
Looks like it went away in Python 3.4.
* :meth:`object.__format__` no longer accepts non-empty format strings, it now
raises a :exc:`TypeError` instead. Using a non-empty string has been
deprecated since Python 3.2. This change has been made to prevent a
situation where previously working (but incorrect) code would start failing
if an object gained a __format__ method, which means that your code may now
raise a :exc:`TypeError` if you are using an ``'s'`` format code with objects
that do not have a __format__ method that handles it. See :issue:`7994` for
background.
Should ours work if an empty string is passed?
There was a problem hiding this comment.
I fixed this by making the test Python version dependent.
This fixes gh-7978, by making the array behavior match the scalar behaviour
The behavior for other sized arrays is left unchanged, pending discussion in gh-5543
Similar to #9201 if you squint, but hopefully not controversial.