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

Refactor epoch to keep time in its own time scale #280

Merged
merged 38 commits into from
Apr 24, 2024

Conversation

ChristopherRabotin
Copy link
Member

Reopening #241

gwbres and others added 30 commits May 27, 2023 14:44
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Co-authored-by: Chris <ChristopherRabotin@users.noreply.github.com>
Co-authored-by: Chris <ChristopherRabotin@users.noreply.github.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Removed deprecated module
I think there is a bug in the Gregorian initialization because it does account for the hour in the reference epoch.

The gregorian initialization and formatter should account for the reference epoch of the time scale.
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Guillaume W. Bres and others added 8 commits September 3, 2023 11:16
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
{:?} for an Epoch now shows the duration in the associated timescale.

Signed-off-by: Guillaume W. Bres <guillaume.bres@bertin.group>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
@ChristopherRabotin
Copy link
Member Author

Hi @gwbres I have some design questions for you.

First, importantly, I think that we should merge this as part of version 4.0 and I'll take this opportunity to clean up the code from the what is deprecated, refactor the modules to make them more maintainable (it isn't OK to have 4000 lines in a single module, I just added things continuously and never cleaned this up), and general chores.

Secondly, here are some questions:

  1. In the new implementation proposed here (which I like a lot), the to_time_scale function currently only performs the conversion between time scales if the original time scale and the new one are different. I think it makes sense that we should avoid computations if we can. But shouldn't we expect the behavior of the code to be the same whether or not we perform this computation? My first thought says "yes" but I'm having second thoughts so I'm interested in what you think. For example, in the ts_over_leap_second time series test, the result is different if I add && false to line 339 of epoch.rs or not.
  2. I'll take advantage of this PR to also fix J1900 and J2000 references mix up, causing older Julian dates to be wrong #282 raised in the discussions. Any objection to that?

That's all for now. But this time, I really want to pick up this work because the list of issues open keeps growing and I want to resolve them.

@ChristopherRabotin
Copy link
Member Author

Moved to #289

@ChristopherRabotin ChristopherRabotin merged commit 729160d into nyx-space:master Apr 24, 2024
5 of 29 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