MAINT: finfo() / iinfo() input/output review#143
Conversation
| if isinstance(type, Array): | ||
| np_type = type._dtype._np_dtype |
There was a problem hiding this comment.
This should be tested in array-api-tests
There was a problem hiding this comment.
Do I take it you're up to adding that test https://github.com/data-apis/array-api-tests/blob/c48410f96fc58e02eea844e6b7f6cc01680f77ce/array_api_tests/test_data_type_functions.py#L151 :-).
| ) | ||
| if not isinstance(other, DType): | ||
| return NotImplemented | ||
| return False |
There was a problem hiding this comment.
You do not want Python to try other.__eq__(self)
There was a problem hiding this comment.
Would be nice to run scipy tests against this branch. I doubt any of the behaviors prohibited here is relied on, but we've been surprised more than once, so...
| def test_finfo_iinfo_wrap_output(): | ||
| """Test that the finfo(...).dtype and iinfo(...).dtype | ||
| are array-api-strict.DType objects; not numpy.dtype. | ||
| """ | ||
| # Note: array_api_strict.DType objects are not singletons | ||
| assert finfo(float64).dtype == float64 | ||
| assert iinfo(int8).dtype == int8 |
There was a problem hiding this comment.
This also ought to be in array-api-tests, TBH
| @pytest.mark.parametrize("func,arg", [(finfo, float64), (iinfo, int8)]) | ||
| def test_finfo_iinfo_output_assumptions(func, arg): | ||
| """There should be no expectation for the output of finfo()/iinfo() | ||
| to be comparable, hashable, or writeable. |
There was a problem hiding this comment.
...or serializable, or have a __dict__, or being targetable by weakref, and probably a few more.
These I think are the important ones.
There was a problem hiding this comment.
Didn't bother testing against them behaving like namedtuples because numpy ones aren't anyway
ev-br
left a comment
ev-br
left a comment
There was a problem hiding this comment.
Looks reasonable, would be nice to double-check it doesn't break scipy though.
| if isinstance(type, Array): | ||
| np_type = type._dtype._np_dtype |
There was a problem hiding this comment.
Do I take it you're up to adding that test https://github.com/data-apis/array-api-tests/blob/c48410f96fc58e02eea844e6b7f6cc01680f77ce/array_api_tests/test_data_type_functions.py#L151 :-).
| ) | ||
| if not isinstance(other, DType): | ||
| return NotImplemented | ||
| return False |
There was a problem hiding this comment.
Would be nice to run scipy tests against this branch. I doubt any of the behaviors prohibited here is relied on, but we've been surprised more than once, so...
| """ | ||
| match = "must be a dtype or array" | ||
| with pytest.raises(TypeError, match=match): | ||
| finfo("float64") |
There was a problem hiding this comment.
Got to admit I do miss the ability to use dtype=float. Nothing to do about it though.
Good news: this branch doesn't cause regressions in scipy |
|
Thanks Guido! Is it more self-flattering to think of ourselves as an Achilles or self-deprecating to think of a no-bug state as a turtle :-). |
|
2 participants
finfo/iinfoaccepts strings? #138iinfoandfinfodo not acceptArrays as input #116