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 a second Time implementation using integer times #46

Merged
merged 5 commits into from
Sep 14, 2023
Merged

Conversation

spenczar
Copy link
Contributor

It's probably time to really consider this.

This is a parallel implementation, not an instant replacement, because it doesn't use the same column names. We'd migrate everything, and then delete the old coordinates.time.

Some decisions:

  • Fixed epoch of MJD. Unix epoch, or JD epoch, or anything else really would have been fine. But MJD is something we're familiar with and can spot unusual values in.
  • 64 bits for both days and day-fractions, which is a bit excessive. We could do 32-bit days and still cover ±~6 million years. But I don't know, more range might be good? I'd reconsider it.
  • All days are treated as having 86,400 seconds. For a UTC-scaled time, this is not correct; leap second days are off by 1. This affects the calculations of the fractional day part, which is consequential for MJD calculations and astropy conversions. But this is the same behavior as SOFA (so it's the same as in Astropy - so we may never notice it). It's a rare occurrence and the maximum error is 1s.

Not implemented yet:

times_equal = pc.equal(self.seconds(), nanos // 1_000_000_000)
else:
raise ValueError(f"Unsupported precision: {precision}")
return pc.and_(days_equal, times_equal)
Copy link
Member

@moeyensj moeyensj Aug 21, 2023

Choose a reason for hiding this comment

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

Defining the precision of a match with an external value is lovely.

@moeyensj
Copy link
Member

@spenczar Is there additional functionality you wanted to add before we merge this in?

@spenczar
Copy link
Contributor Author

The only thing would be astropy-free scale conversions but I don't think that's necessary, we can merge this I think.

@spenczar spenczar merged commit 76243db into main Sep 14, 2023
5 checks passed
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