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

epoch::from_duration slight improvements #238

Closed
wants to merge 1 commit into from

Conversation

gwbres
Copy link
Collaborator

@gwbres gwbres commented May 27, 2023

Hello @ChristopherRabotin,

this is an inquiry that might get pruned.
i'm trying to see if there are any possibilities to simplify some conversions and construction ops.

Right now the only thing I achieved is a slightly tighter Epoch::from_duration and use it everywhere.

Inquiring #237 and possible traits will most certainly have more impact

@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.09 ⚠️

Comparison is base (4d0f6f0) 81.09% compared to head (c8648ef) 81.01%.

Additional details and impacted files
@@              Coverage Diff              @@
##           4.0.0-dev     #238      +/-   ##
=============================================
- Coverage      81.09%   81.01%   -0.09%     
=============================================
  Files             16       16              
  Lines           3783     3760      -23     
=============================================
- Hits            3068     3046      -22     
+ Misses           715      714       -1     
Impacted Files Coverage Δ
src/epoch.rs 88.94% <100.00%> (-0.11%) ⬇️
src/timescale.rs 97.43% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gwbres gwbres force-pushed the 4.0.0-dev-ts branch 7 times, most recently from 463b14f to b0951c4 Compare May 27, 2023 12:37
@gwbres gwbres changed the title initiate the timescale trait epoch::from_duration slight improvements May 27, 2023
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
/// Creates an epoch from given duration expressed in given timescale.
/// In case of ET, TDB Timescales, a duration since J2000 is expected.
#[must_use]
pub fn from_duration(new_duration: Duration, ts: TimeScale) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If #237 architecture is adopted, this can simply become:

pub const fn from_duration(duration: Duration, ts: TimeScale) -> Self {
    Self {
        duration,
        time_scale: ts
    }

Then the conversion to/from different time scales only happens when another time scale is requested. For example, if someone did this:

let e = (366.days() + 1.hours()).ephemeris_time() // Create an ephemeris time epoch that's 2001-01-01T01:00:00 ET (because the ET reference is 01 Jan 2000)
let e_utc = e.into_time_scale(TimeScale::UTC) // This is when the computation between time scales happen

Note that I think into_... works here given the Rust naming convention.

As I type this, two other things come to mind:

  1. I think it would make sense for Week to be a Unit so one could do 5.weeks() + 6.hours(): a week is always exactly 7 days, and I think we didn't add Week to Unit because it would have been a breaking change
  2. The reference epoch should be defined for all TimeScales. The issue still comes with modifiers since there may be some use cases of 365.days().mjd_ephemeris_time() where the reference epoch is Modified Julian Days... but that adds complexity and rigidity because people can't easily define new reference epochs... Maybe something 365.days().ephemeris_time_from(REF_EPOCH) where ref_epoch is also an epoch that's added to the ephemeris_time reference epoch ... I don't know what's best!

@gwbres
Copy link
Collaborator Author

gwbres commented May 28, 2023

moved to dev-gh-237 since i'm inquiring this topic specifically

@gwbres gwbres closed this May 28, 2023
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