Refactor Epoch to its own time scale

[gwbres]

Trying to see how complex answering to #237 would be.

• Renamed Epoch::j1900_duration
• Introduced/Renamed Epoch::to_time_scale : follow naming conventions
• Epoch::with_time_scale is just a construction helper (follows naming conventions too)
• (+) and (-) ops first invoke a rhs.to_time_scale(self.ts) so the computation is feasible
• We need common ground for cmp and eq ops, but i'm not sure it really makes sense to go this far.
  Maybe strict equality/comparison between both self.duration and self.ts is enough. Right now, the current code produces a funny behavior where .eq() fails for two identical datetimes:

  left: `2020-01-01T00:00:37 UTC`,
 right: `2020-01-01T00:00:37 UTC`test asn1der::test_encdec ... ', src/epoch.rs
FAILED: 3241:5

• Epoch::to_time_scale(TimeScale::TDB) and other time scales that need to refer to J2000 may not be feasible if we come from a datetime that is not past J2000 (i marked a TODO)
• The ton of test failures comes from
  • Epoch::maybe_from_gregorian possibly broken, although I kept the previous logic and it should be closed to working
  • Epoch::compute_gregorian, used all over the place, might no longer work because self is no longer referenced to J1900

[gwbres]

Ok i fixed several issues by auto deriving Eq and Hash.

NB: it is considered bad practice to implement Hash yourself if you don't implement Eq yourself or vice-versa.
Unfortunately I cant' recall the reason for that.
Also, current lib hashes the inner duration, but not the inner time scale: this will probably lead to serdes issues. Auto derivation takes care of that.

I moved to auto derivation for Eq and Hash.

As far as PartialEq and Cmp ops go, we still implement ourselves, because we need to convert to identical time scales for an exact duration comparison

[ChristopherRabotin]

This is a superb start and exactly what I had envisioned!

[ChristopherRabotin]

Yes, I think that's correct. If I understand this correctly, it means that only ET and TDB have a different "tick" rate then the other time scales, i.e. one second in {TAI, UTC, GPS, GST, TT, ...} is one second in all of the other time scales, apart for TDB and ET. I think that is correct.

[gwbres]

OK so most errors come from faulty From_gregorian and Compute_to_gregorian functions.
Could you help me out on those ?

I think I'm on the right track for Self::from_gregorian to which we now must add a notion of time scale.
I test it with cargo test gpst if you read the first few lines of tests::epoch.rs:gpst, we have a simple use case.

In V3, everything is referenced to TAI_J1900. So we have += 1900 hardcoded at some point, in from_gregorian().
I managed to get the timescale reference offset correctly.
With this, for example, i'm able to have cargo test test_serdes to work.
But for some reason, the equality of this simple 1 second into GPST test is failling. It's probably due to the equal operator implementation

[ChristopherRabotin]

Sorry I dropped the ball on this, I totally forgot you were working on it. Let me see what I can do.

[ChristopherRabotin]

The tests are nearly there, but I think that the one test that fails actually shows an error nested deep in the code, and I can't seem to figure out how to fix it yet.

The conversion from a J1900 referenced epoch to a J2000 epoch is wrong by twelve hours. It so happens that the difference between these two references is indeed twelve hours: J1900 starts a noon but J2000 starts at midnight.

What I don't understand is that the computation to/from ET/TDB (which are J2000 based) is correct according to all of the validation test cases. But when we compute the Gregorian time, the twelve hour difference is visible. So I don't know what is going on.

[gwbres]

The tests are nearly there, but I think that the one test that fails actually shows an error nested deep in the code

Thank you for your time, I'll take a look into this next week end, see you

[ChristopherRabotin]

@gwbres I might be able to pick up this work in September. Have you had time to review the issue I found ? I really hope that it's me misunderstanding something because the code is very close to being complete if the time difference is not a bug.

[ChristopherRabotin]

Thanks for all your help here, I'll run the tests locally soon. Are the UTC and GNSS time scales nearly working? I can handle the TT, ET, TDB.

[gwbres]

I don't understand why the library always prints UTC, it is very confusing.
For example epoch = Epoch::from_gpst_seconds(1.0) prints 1980-01-06:01 UTC, but we want 1980-01-06:01 GPST, basically the gregorian date with emphasis of which time scale it is based on ?

This is due to the Display and Debug implementations. Although I would be OK with a diffrent debug behavior, I don't think the current Display behavior is right. What is your opinion on that ? it's confusing when dealing with a specific time scale, to see something expressed in UTC

Are the UTC and GNSS time scales nearly working? I can handle the TT, ET, TDB.

Hard to say. For me, the following needs to be solved :

• How an epoch is displayed
• To and From gregorian() have issues, which creates problems in places you would not expect.
  For me it's ok to have gregorian datetimes in timescales other than UTC
• PartialEq : are we ok on this ?
• to_time_scale() and its relationship with to_xxxx_duration() and .leap_seconds().
  For me, we should have a to_time_scale(TAI) that serves as the foundation to all other conversions.
  At least, that's what I tried to do

[ChristopherRabotin]

I'm happy changing the behavior of Display to display the epoch in its own time scale. Maybe Debug could show the Duration in that time scale, e.g. 3455 days 2 h 15 ns since GPS epoch. I think that provides useful debugging information.

For the to_time_scale(), converting via TAI is what we essentially have for version 3.x and it makes things complicated sometimes. I think that there was a commit a few months ago on this branch where I did a pretty good implementation of that function and I think it worked for most cases.

What's the issue with PartialEq ? I think that this makes sense, no?

//// If one of the two time scales does not include leap seconds,
//// we always convert the time scale with leap seconds into the
//// time scale that does NOT have leap seconds.

[gwbres]

I think that there was a commit a few months ago on this branch where I did a pretty good implementation of that function and I think it worked for most cases

Reverting to that state (now)

[gwbres]

What's the issue with PartialEq ? I think that this makes sense, no?

I just don't see why having a case specific to Leap Seconds,
while we have an else {} branch that will manage it anyway :

 266             if self.time_scale.uses_leap_seconds() != other.time_scale.uses_leap_seconds() {
 267                 if self.time_scale.uses_leap_seconds() {
 268                     self.to_time_scale(other.time_scale).duration == other.duration
 269                 } else {
 270                     self.duration == other.to_time_scale(self.time_scale).duration
 271                 }
 272             } else {
 273                 // Otherwise it does not matter
                          // --> this can manage the leap seconds
 274                 self.duration == other.to_time_scale(self.time_scale).duration
 275             }

[gwbres]

Maybe Debug could show the Duration in that time scale, e.g. 3455 days 2 h 15 ns since GPS epoch. I think that provides useful debugging information.

I think that is a great idea

[gwbres]

Sorry some of my modifications yesterday were wrong. I reverted everything to the best state we had..

My understanding is, we're correct on compute_gregorian, and almost correct in maybe_from_gregorian().
It looks like, in maybe_from_gregorian, we account for the TimeScale starting year offset correctly, but not for offsets within that year. For example, it does not work for GPST and GAL that were not started on a January 1st

[gwbres]

@ChristopherRabotin

I just pushed a commit that modifies the std::fmt::Debug behavior for Epoch.

It now prints the duration in the associated time scale.

I think it's a very good idea you had and we should not forget it.
It might make things much easier when debugging, especially debugging casts and calculations.

std::fmt::Display is the one intended for the end user.
I also noted this commit did not induce more test failures

[gwbres]

Hello @ChristopherRabotin

Can you please review what's being tested in tests/epoch.rs fn test_timescale_recip ?

There is something terribly wrong with this test, either how it's designed or how it's operated.

How can a duration ("from_dur") built from a different timescale, be the same as a UTC duration.
But I get what you're trying to do. You're trying to see if a reciprocal cast leads to a strictly identical duration.
But Epochs and Durations are two different things. We need to compare durations here, not Epochs.

I just pushed a commit that modifies the PartialEq behavior (see Epoch::PartialEq), and this test is the only one that used to pass, now fails due to that modification. Might be a good thing ?

Any chances to make progress on this topic in the near future?

[ChristopherRabotin]

I think that this test is obsolete now with the new design: it used to be important because we would convert from the initialization time scale to TAI every time, so the test would make sure that if we initialized at Epoch X in time scale T, when it was converted to TAI and then requested back in T, then the same epoch X would be returned.

I think this test can be deleted now.

[ChristopherRabotin]

I'm OK with the changes to PartialEq. The previous implementation was trying to be clever, but maybe the better use case is to ask the user to convert both epochs to the same time scale before comparing them.

[ChristopherRabotin]

@gwbres I have a bit more clarity on my schedule, and I think I can pick up the work on this in February. I've also been thinking more about this, and I now think that this change can be (and should be) merged into the same 3.x version of hifitime. Although in theory any changes to the enums are considered breaking changes, I think it's unlikely that users perform a match on the time scale, and as such, the changes introduced in what we initially called 4.0.0 are only "potentially but unlikely" breaking changes. Keeping everything in version 3.x would mean that we have to keep the older functions available, and just mark them as deprecated.

What do you think?

[gwbres]

It sounds good to me.
The only thing to be very careful with this new definition, is the Eq operator.
Auto derivation is not possible because auto derivation :

#[derive(PartialEq)]
pub struct Epoch {
    Duration,
    TimeScale

would return true when dur(a) == dur(b) && timescale(a) == timescale(b).
This behavior is very wrong because it is possible to represent the same "time" with different durations in different timescales.
I'm very used to working with GPST for example, let's take the Midnight UTC example, GPST is late by the amount of leap second, so midnight UTC and 23:59:23 GPST are the same instant.
Conclusion : Eq needs to be manually implemented and requires the conversion into a common timescale

[ChristopherRabotin]

Good reminder!

…

On Wed, Jan 3, 2024, 15:02 gwbres ***@***.***> wrote: It sounds good to me. The only thing to be careful with this new definition, is the Eq operator. Auto derivation is not possible because auto derivation : #[derive(PartialEq)]pub struct Epoch { Duration, TimeScale would return true when dur(a) == dur(b) && timescale(a) == timescale(b). This behavior is very wrong because it is possible to represent the same "time" with different durations in different timescale. I'm very used to working with GPST for example, let's take the Midnight UTC example, GPST is late by the amount of leap second, so midnight UTC and 23:59:23 GPST are the same instant. Conclusion : Eq needs to be manually implemented and requires the conversion into a common timescale — Reply to this email directly, view it on GitHub <#241 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEZV2BGFZPLV7XPK5AE2ATYMXIQ7AVCNFSM6AAAAAAYRXUYJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZWGAZDSMRYGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[gwbres]

I don't know if continuing this branch is the way to go, it is one possibility.
Once the Epoch redesign is stabilized, I can have my tools use a local version of a PR in question.
We use the library extensively, so it will be another good indication

[ChristopherRabotin]

This should not have been closed ! I didn't expect that delete the 4.0.0-dev branch would close this PR. Can you re-open it directly against master?

[gwbres]

Sure, I will take care of this in the next few days Guillaume W. Bres Software engineer ***@***.***> Le sam. 13 janv. 2024 à 19:48, Chris ***@***.***> a écrit :

…

This should not have been closed ! I didn't expect that delete the 4.0.0-dev branch would close this PR. Can you re-open it directly against master? — Reply to this email directly, view it on GitHub <#241 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXPAQYRORBFLEM4BUW6VI3YOLJG5AVCNFSM6AAAAAAYRXUYJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJQGY4TOOJYGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>