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

Add Times.unique(), .from_mjd(), and .from_jd() #43

Merged
merged 3 commits into from
Aug 18, 2023
Merged

Conversation

moeyensj
Copy link
Member

No description provided.

@moeyensj moeyensj requested a review from spenczar August 18, 2023 13:03
assert times.scale == times_astropy.scale
np.testing.assert_allclose(
times.mjd().to_numpy(), times_astropy.mjd, rtol=0, atol=1e-9
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was originally np.testing.assert_equal but it would fail with:

AssertionError: 
E           Arrays are not equal
E           
E           Mismatched elements: 981 / 1000 (98.1%)
E           Max absolute difference: 2.32830644e-10
E           Max relative difference: 4.64916676e-15
E            x: array([50000.      , 50010.01001 , 50020.02002 , 50030.03003 ,
E                  50040.04004 , 50050.05005 , 50060.06006 , 50070.07007 ,
E                  50080.08008 , 50090.09009 , 50100.1001  , 50110.11011 ,...
E            y: array([50000.      , 50010.01001 , 50020.02002 , 50030.03003 ,
E                  50040.04004 , 50050.05005 , 50060.06006 , 50070.07007 ,
E                  50080.08008 , 50090.09009 , 50100.1001  , 50110.11011 ,...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing astropy from the unit tests didn't resolve this issue. If we can roundtrip through MJD then I think we should drop from_mjd.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is a fundamental limit of floating point time in a single 64-bit array.

64 bit floats can represent at most 15 decimal digits exactly. Thats because they have 53 bits in their exact part, and 11 bits for an exponent. 53 binary digits is 15 decimal digits (log10(2^53)).

An MJD for the typical values spends 5 decimal digits on the day part, so at best we can hope for an atol of 1E-10. Evidently we lose a bit or two in the conversion to and from JD, so in practice we get more like 1E-9. We might be able to improve that with a tremendous amount of care but we will never surpass 1E-10 absolute tolerance for MJD in [10,000, 99,999], I am afraid.

Copy link
Contributor

@spenczar spenczar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but please add some comments to the docstrings warning that to/from MJD is only accurate to about 0.1ms.

JD might be worse since it spends so many bits on the day part. Maybe document that fact as well.

Ah, floating point time, why do you exist?

@moeyensj moeyensj merged commit a48a6f5 into main Aug 18, 2023
1 check passed
@moeyensj moeyensj deleted the jm/unique-times branch August 18, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants