nyx-space / hifitime

A high fidelity time management library in Rust
Mozilla Public License 2.0
355 stars 18 forks source link

Simplify Epoch initializations from time scales #229

Open ChristopherRabotin opened 1 year ago

ChristopherRabotin commented 1 year ago

The first PR for version 4 adds non-exhaustive to the TimeScale enum. This gives us plenty of flexibility to add additional time scales, like #191. However, we also currently have an Epoch::from_{time_scale}_[days,seconds,nanoseconds]() initializer. That's a lot of code, and a good chunk of it is duplication.

With version 4, we have the opportunity to refactor a lot of these initializers.

I recommend that we remove the Epoch::from_{time_scale}_[days,seconds,nanoseconds]() initializers and instead allow std::ops::Mul between a duration and an time scale for the initialization. The TimeUnits trait, included in the prelude, allows simple initialization of durations, and so does the Units enum.

For example: Epoch::from_utc_seconds(1234.5) would become 1234.5.seconds() * TimeScale::UTC. We could also add a TimeScaleTrait to further this initialization as 1234.5.seconds().utc().

The main limitation of this approach is for modified time references like (modified) Julian days: these currently have their own initializers, so we'd need to keep a generic initialization like Epoch::new(duration: Duration, time_scale: TimeScale, modifier: Option<Modifier>) where Modifier would be Julian, modified Julian, and potentially other ones like the NASA GMAT MJD (which is off by hundreds of years compared to the real MJD).

My slight concern here is that the idea of a "multiplication" between a duration and a time scale does not strictly speaking make any kind of sense. I mostly like the idea of overloading an operator for the initialization instead of requiring the user to use a constructor: this is especially relevant in "script like" usages of the library. Moreover, the multiplication makes it easy to integrate in Python and to keep a similar API: >>> 1234.5 * Unit.Second * TimeScale.UTC.

@gwbres , what do you think ?

gwbres commented 1 year ago

Hello @ChristopherRabotin,

As far as the code quantity problem goes (related to timescales), the TimeScale trait is the way to go. Constructions like Epoch::from_seconds(1234.5).utc() because a Duration would implement the traits is easy, elegant, and removes 80% of existing methods.

Then, regarding the "Modified" dates, I don't see any problems with keeping existing methods to make things easier. And if we want to also reduce their code quantity, maybe a dedicated Trait, like DateModifier, Modifier, insert any better suited name here, might prove more handy too

ChristopherRabotin commented 10 months ago

Since these changes will be made in version 3.10, backward compatibility is required. The previous initializers should be changed to be marked deprecated with alternative recommendations.

ChristopherRabotin commented 1 month ago

Changes in initializers will happen in a future version 5.0.