podusowski / rhorizons

Access JPL's Horizons system from Rust
MIT License
2 stars 3 forks source link

Uom - SI based parsing #16

Closed NiklasVousten closed 1 year ago

NiklasVousten commented 1 year ago

This allows to use SI-based ephemeris data using the uom crate. #4 Feature was implemented with feature flags, so si will allow SI units.

Uom units are f32 based. It might be nice to have f64 for SI and non-SI format. Maybey a restructuring of the repo to have a similar structure like uom? With the addition of a f64 feature flag with f32 as default.

Version is not yet bumped.

podusowski commented 1 year ago

I don't really like that everything is just duplicated now. :)

How about putting all types into separate, feature controlled modules? Something like:

#[cfg(feature = "si")]
mod units {
  type Velocity = uom::blabla;
}

#[cfg(not(feature = "si"))]
mod units {
  type Velocity = f64;
}
NiklasVousten commented 1 year ago

While this seems to work, and only requires minor changes with parsing, the docs are dependent on the feature flag. So depending which flag is used for the creation of the documentation, the docs will be incorrect for the other option.

For now I keep the current approach, but will try to minimize code duplication.

NiklasVousten commented 1 year ago

Currently there is no dedicated parser for the SI units. the client functions just use the core functions and then converts the default units to SI. A similar approch could be used for a dedicated parse, but is not useful currently, and thus only present as a comment in si::ephemeris. There is still a small amount of code duplication, but I believe it is much better now.

Propsed approach for f64:

pub struct EphemerisVectorItem<T> {
    pub time: DateTime<Utc>,
    pub position: [T; 3],
    pub velocity: [T; 3],
}
podusowski commented 1 year ago

Cool, thanks.