Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Float comparison issues with time and quantity #6970

Open
bsipocz opened this issue Dec 12, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@bsipocz
Copy link
Member

commented Dec 12, 2017

The following snippet returns True with astropy 2.0.2, but False on master and 2.0.x (2.0.3.dev19469)

>>> import astropy
>>> print(astropy.__version__)
>>> from astropy.time import Time
>>> import astropy.units as u
>>> start = Time('2016-02-06 03:00:00')
>>> duration = 2*u.hour
>>> end_time = start + duration
>>> end_time - duration == start
2.0.2
True
>>> import astropy
>>> print(astropy.__version__)
>>> from astropy.time import Time
>>> import astropy.units as u
>>> start = Time('2016-02-06 03:00:00')
>>> duration = 2*u.hour
>>> end_time = start + duration
>>> print(end_time - duration == start)
>>> print(end_time - duration)
>>> print(start)
3.0.dev20874
False
2016-02-06 03:00:00.000
2016-02-06 03:00:00.000
@mhvk

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

This is weird. Same problem on master, but

(end_time - duration - start).to(u.ns)
# <Quantity 0.0 ns>

It clearly is some kind of rounding problem, though:

In [11]: start.jd1, start.jd2
Out[11]: (2457425.0, -0.375)

In [12]: (end_time - duration).jd1, (end_time - duration).jd2
Out[12]: (2457425.0, -0.37500000000000006)
@mhvk

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

Actually, it is a bit of a stranger rounding problem: since hours are not a constant fraction of a day, the time is converted to TAI before the duration is subtracted, and then converted back to UTC. The problem doesn't happen if we start with TAI:

In [15]: start = Time('2016-02-06 03:00:00', scale='tai')

In [16]: end_time = start + duration

In [17]: end_time - duration == start
Out[17]: True

I think the underlying cause of the change is #6653 - I think the problem it fixed is greater than this one, though.

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

p.s. integer for Time is days; 2 hours = 1/12 of a day, so not exactly representable in binary.

@bsipocz bsipocz changed the title Float comparison issues with time and "integer" quantity Float comparison issues with time and quantity Dec 12, 2017

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

A bit further investigation: it is a round-trip error in converting from utc to tai:

start = Time('2016-02-06 03:00:00')
start == start.tai.utc
# False
start.jd2
# -0.375
start.tai.utc.jd2
# -0.37500000000000006

Note that it is just unlucky rounding. The same happens in 2.0.2 if I ensure input is rounded as it normally is:

start = Time(2457424.625, format='jd')
start.jd1, start.jd2
# (2457425.0, -0.375)
start == start.tai.utc
# False
start = Time('2016-02-06 03:00:00')
start.jd1, start.jd2
# (2457424.5, 0.125)
start == start.tai.utc
# True
@mhvk

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

Checking even further, it boils down to erfa not quite round-tripping:

import astropy._erfa as erfa
erfa.taiutc(*erfa.utctai(2457424.5, 0.125))
# (array(2457424.5), array(0.125))
erfa.taiutc(*erfa.utctai(2457425.0, -.375))
# (array(2457425.0), array(-0.37500000000000006))

I can make the particular problem go away with a small change in jd2cal. Arguably, it is now an upstream problem.

@bsipocz

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2017

@mhvk - Thanks for investigating. Take away for downstream will be to adjust those tests as all of it is upstream from astroplan.

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

Yes, I think any tests like this on time should test closeness (<1ns is reasonable) rather than equality.

@eteq

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

Yes, I think any tests like this on time should test closeness (<1ns is reasonable) rather than equality.

Actually I'm not sure I agree there. You never know when someone really wanted to check that it's closer than 1 ns...? Same reason why we don't do equality in coordinates et al - "equals" really is ambiguous here if you don't mean literally bit-wise equality...?

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

@eteq - the comment was about tests on round-tripping. Since this depends on float rounding working the way we want, I don't think we can guarantee equality, so all I am suggesting is that we do not use __eq__ in tests, not that we adjust the meaning of __eq__!

@Juanlu001

This comment has been minimized.

Copy link
Member

commented May 13, 2018

Maybe it's because of #6653?

@Juanlu001

This comment has been minimized.

Copy link
Member

commented May 13, 2018

Sorry for the noise, @mhvk said exactly that - should have "Ctrl+F"-ed.

@mhvk

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Looking at older issues, I think the summary here is whether it is worth trying to improve round-tripping for ERFA/SOFA, and push that upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.