BUG: allow for precision > 17 in longdouble repr test #8656
Conversation
numpy/core/tests/test_longdouble.py
Outdated
| # We will only see eps in repr if within printing precision. | ||
| # Printing precision defined as 17 in scalartypes.c.src | ||
| expected = 1.0 if LD_INFO.precision > 17 else o | ||
| assert_equal(np.longdouble(repr(o)), expected, | ||
| "repr was %s" % repr(o)) |
There was a problem hiding this comment.
Wait, isn't the bug here that the repr is capped at a precision of 17 when it shouldn't be?
There was a problem hiding this comment.
Also, scalartypes.c.src seems to cap it at 20, not 17, unless longdouble and double are the same
There was a problem hiding this comment.
Yes, you're right, I was just looking at that code. Can correct the comment and constant, but the question is, do we really want - for example - 31 digits of precision for np.longdouble (that's what double double gives, on PPC64). If so, we'd have to adapt the scalartypes.c.src code to detect the different types of longdouble.
There was a problem hiding this comment.
I think it's desirable that repr roundtrips properly, and I don't think that changing the test to allow the failure here is the right solution. So yes, on PPC64 I think that it should print 31 digits for repr - there's always str for the sane-length output.
Obviously it's not desirable use 31 digits on platforms that don't support it, but as a quick fix, I think that 20 should be become 33 (precision +2 used elsewhere).
Adapting scalartypes.c.src to choose more intelligently is an obvious next step.
973f84b to
e9061a8
Compare
|
Pointing to now-hidden discussion : #8656 (comment) The question is - how much do we care about the round-trippiness of repr for longdouble? If we do care about that then: a) we'd get 33 digits of repr precision on PPC64, and 36 on SPARC (real float128) and |
|
Side-note: make a PR with just the |
|
My vote would be to give up on struct repr roundtrip correctness, at least for now, because:
|
As far as I remember, |
|
OK - happy to withdraw that part of the argument. |
|
I'm not convinced the workaround is correct though. I think that if the precision is too high, the test should be skipped altogether, rather than verifying that it does not roundtrip |
|
How about: Then, if we want to increase the repr precision later, the test will be triggered automatically. |
|
That sounds pretty good to me, although I might still be tempted to mark it as a skipped test somehow rather than it no-opping |
e9061a8 to
40e123b
Compare
|
OK - I pushed up a version where we skip the test when there isn't enough repr precision. |
numpy/core/tests/test_longdouble.py
Outdated
| a = np.array([o, o, o]) | ||
| assert_equal(a[1], o) | ||
|
|
||
|
|
||
| # Conversions string -> long double | ||
|
|
||
|
|
||
| @dec.skipif(LD_INFO.precision >= len(repr(np.longdouble(0.1))) - 2, |
There was a problem hiding this comment.
Probably better doing:
repr_precision = len(repr(np.longdouble(0.1)))
# +2 comes from <the right lines in scalar_types>
@dec.skipif(LD_INFO.precision + 2 >= repr_precision)Precision of repr set to 22 when longdouble != double, but this isn't enough for double double (precision 33) or float128 (precision 34). If repr precision is not high enough to show eps value, skip the test. Closes numpygh-7940.
test_longdouble had two tests called test_fromstring_foreign, so the second was overwriting the first. Rename the first so test gets run.
40e123b to
cd2446c
Compare
|
Made that change. Also (sorry) - I re-enabled an unrelated test that was being overwritten. |
|
|
|
Looks good to me. I doubt anyone is actually going to be able to confirm those test results on SPARC or PPC64el, so I'll take your word for it |
Precision of repr print unconditionally set to 20, for longdoubles != double. Allow for this
in repr round-trip test for longdouble.
Closes gh-7940.