-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refine time representations #44
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
helgee
approved these changes
Jan 18, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo naming nitpicks.
helgee
reviewed
Jan 24, 2024
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
+ Coverage 97.69% 97.84% +0.15%
==========================================
Files 40 42 +2
Lines 16273 17322 +1049
==========================================
+ Hits 15898 16949 +1051
+ Misses 375 373 -2 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes several updates to the structure of the
time
package and its associated structs.Rationale
Time
variant)Design
UTC
dates::Time
is nowutc::UTC
, and is intended only for use as an IO time to be converted to one of the continuous timescales. This gives the user the freedom to specify a time in human-readable format (to attosecond precision) while excusing us from worrying about time zones other than UTC. The correctness of the user-providedUTC
with regard to leap seconds is explicitly the user's responsiblity.utc::UTC
functions identically to the formerdates::Time
, but has some internal structural changes that leverage the type system to ensure that each time field falls within a valid range.ContinuousTime
epochs::RawEpoch
is nowcontinuous::ContinuousTime
to emphasise that it is the basis of all continuous timescales. A comment has been added explaining its intended use.Internally, the
attoseconds
field is now au64
and the sign of theContinuousTime
(i.e. before or after some arbitrary epoch) is determined exclusively by the sign of theseconds
field.Delta
A continuous time
Delta
struct has been added, along with operator overloads for adding and subtractingDelta
s fromContinuousTime
s. This is a prerequisite for converting between timescales.Time
epoch::Epoch
is nowcontinuous::Time
, reflecting modern usage of the word "epoch".Rust's type system doesn't allow us to require a specific enum variant as a function parameter (i.e. a parameter may be of type
Time
but cannot be of typeTime::UT1
). For this reason, each variant now embeds a dedicated struct of the corresponding time type, e.g.UT1
,TCB
, etc. This is required to support functions that require a time in a specific timescale to do so in a type-safe way without requiring a match statement to identify the specific time variant that has been passed by the user. The match statement approach is problematic: since conversion between arbitrary time scales often requires non-constant inputs (e.g. 𝚫UT), a function accepting an arbitraryTime
variant would also have to accept all possible parameters required to convert between any variant and the required timescale for the current calculation.