Fixes for some of the problems uncovered on ARM bigendian/quad precision#9672
Fixes for some of the problems uncovered on ARM bigendian/quad precision#9672pllim merged 4 commits intoastropy:masterfrom
Conversation
|
cc @aarchiba for the time tests. |
| assert np.all(out == ufunc(q.value)) | ||
|
|
||
| # Ignore RuntimeWarning raised on Windows and s390. | ||
| @pytest.mark.filterwarnings('ignore:.*invalid value encountered in sign') |
There was a problem hiding this comment.
I thought we agreed in #7928 to not blindly ignore this warning as such so we'll know when Numpy fixes its bug upstream? Isn't this a step backward?
There was a problem hiding this comment.
Yes, that was when it was windows only... now I just don't know anymore where it could fail, so decided we might as well ignore it, as it really isn't our problem. (But I admit to being too lazy to check how to find out all possible places where this could be wrong...)
There was a problem hiding this comment.
Hey, it turns out I now get these warnings even on my linux box (for numpy-dev), so filtering is OK after all. How nice to have laziness rewarded!
astropy/time/tests/test_basic.py
Outdated
| # Check we're the same to within the double holding jd2 | ||
| # (which is less precise than longdouble on arm64). | ||
| assert np.allclose(t.to_value('mjd', subfmt='long'), | ||
| expected_long, rtol=0, atol=2.**-52) |
There was a problem hiding this comment.
Why not use np.finfo(float).eps rather than hard-coding the 2**-52?
There was a problem hiding this comment.
Will this still work okay when longs have less precision than Time objects, for example when they are 80 or 64 bits?
There was a problem hiding this comment.
64-bits are filtered out, but 80-bit would still work, since the setting of jd2 from it is exact to 2**-52, i.e., the same rounding errors will be made. (And other pieces test that the input longs are different)
| t = Time(mjd_long, format='mjd') | ||
| expected = Time(i, f, format='mjd') | ||
| assert t == expected | ||
| assert abs(t - expected) <= 20.*u.ps |
There was a problem hiding this comment.
should there be Time.eps? Presumably with the value (np.finfo(float).eps*u.day).to(u.ps)?
There was a problem hiding this comment.
I like that idea! Though out of scope here (and not quite sure about implementation).
There was a problem hiding this comment.
What's wrong with creating a class attribute in Time whose value is (np.finfo(float).eps*u.day).to(u.ps)?
There was a problem hiding this comment.
Just that no other class has this attribute, which means it won't be easily discovered - I considered whether we could override np.finfo on Time - but that is not possible.
Anyway, probably best discussed in a separate issue.
There was a problem hiding this comment.
A propos overrides, what would be easy to do is to override np.allclose to work on Time instances; that could have then a reasonably sensible default for atol of a few times the float precision in days.
| assert np.all(out == ufunc(q.value)) | ||
|
|
||
| # Ignore RuntimeWarning raised on Windows and s390. | ||
| @pytest.mark.filterwarnings('ignore:.*invalid value encountered in sign') |
There was a problem hiding this comment.
Is "ignore" correct here? Shouldn't we have well-defined behaviour - like, the sign of +np.inf should be 1? One could test that the sign behaviour agreed with that of the sign of the underlying floating-point number. (It's weird that that is platform-dependent but at least the two should agree.)
There was a problem hiding this comment.
The answers are actually reasonable - and those are still tested. All this does is to filter out the numpy warnings, which we have no control over (and do not occur on all platforms), so that these do not cause test failures.
| assert np.all(out == ufunc(q.value)) | ||
|
|
||
| # Ignore RuntimeWarning raised on Windows and s390. | ||
| @pytest.mark.filterwarnings('ignore:.*invalid value encountered in sign') |
|
|
||
|
|
||
| # Ignore RuntimeWarning raised on s390. | ||
| @pytest.mark.filterwarnings('ignore:.*invalid value encountered in.*') |
There was a problem hiding this comment.
Here too, I think the warning behaviour should be specified rather than simply passing anything.
There was a problem hiding this comment.
Again, note this does not affect test failure, just that we do not fail depending on whether or not this warning is emitted.
|
(sorry for the slightly redundant review comments I had forgotten to hit "submit review") |
|
I think I got all comments - anything left? |
pllim
left a comment
There was a problem hiding this comment.
I am no expert in time but I didn't see any red flags, so merging. Thanks!
|
Thanks, I've quickly rebased #9719 - hopefully it'll now show an all-clear numpy-dev job |
Fixes for some of the problems uncovered on ARM bigendian/quad precision
@bsipocz - I found it easier to just separate out the fixes. Note that these may not be all; in particular, it doesn't get to #9657 but does fix the other problems in #9661.