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

Simplify Epoch initializations from time scales #229

Open
ChristopherRabotin opened this issue May 23, 2023 · 2 comments
Open

Simplify Epoch initializations from time scales #229

ChristopherRabotin opened this issue May 23, 2023 · 2 comments

Comments

@ChristopherRabotin
Copy link
Member

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
Copy link
Collaborator

gwbres commented May 26, 2023

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants