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

Implement spk parser #4

Closed
wants to merge 27 commits into from
Closed

Conversation

matzipan
Copy link
Collaborator

@matzipan matzipan commented Mar 12, 2023

Relates to #2

This is only a parser for the file record so far.

One design choice was to make all the vector types and the strings owned. The alternative would've been to tie the struct to a lifetime and avoid copying the 900 bytes of prenul, ftpstr and postnul. But I decided this would be a minor gain against the downside of having to manage the extra lifetime.

The test validates the values read against the values return by the NASA NAIF spy executable.

Another design choice was to keep the struct field names as close as possible to the ones in the spec, I am ambivalent about this.

Finally, another choice was to leave the locfmt field a string instead of transforming this into an enum. The reason for this is because I kept this data structure relatively close to the binary format (for example by keeping prenul, ftpstr and postnul). I currently can't imagine a need for the user of the daf/spk library to care about these implementation details, so I will keep it this way. In the future, it will be hidden behind an API closer to the julia implementation anyway.

Still to do:

  • Get rid of mod.rs files
  • Parse comment area record
  • Parse descriptor record
  • Parse segment ID record
  • Understand if summary values are correct
  • Implement DAF to SPK segment conversion
  • Take into account endianness when parsing
  • Implement SPK type 2 parser
  • Rework test for SPK type 2
  • Implement position
  • Implement get_segments
  • Fix todos
  • Implement velocity
  • Implement state
  • Benchmark and optimize
  • Validate position result
  • Add task for using two-part doubles for julian dates to improve precision Use two part doubles to improve precision #13
  • Convert initial_epoch and final_epoch to Date from j2000 Integrate ephemeris code with Date code #14

@matzipan matzipan force-pushed the implement_spk_parser branch 2 times, most recently from a5ab074 to 2d0ae4d Compare March 12, 2023 22:20
@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (da4c6b8) 92.40% compared to head (bca9799) 93.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
+ Coverage   92.40%   93.12%   +0.71%     
==========================================
  Files          15       16       +1     
  Lines        5053     5698     +645     
==========================================
+ Hits         4669     5306     +637     
- Misses        384      392       +8     
Files Coverage Δ
crates/lox_core/src/ephemeris/daf_spk/parser.rs 98.75% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@helgee
Copy link
Member

helgee commented Mar 12, 2023

I think I would prefer getting rid of the mod.rs files, otherwise LGTM.

@matzipan matzipan self-assigned this Mar 12, 2023
@matzipan
Copy link
Collaborator Author

I asked @helgee what user-facing API should I implement, he said the following:

start with the low-level interface, i32 for the body ids and two-part Julian Day numbers (f64, f64).

@matzipan matzipan force-pushed the implement_spk_parser branch 3 times, most recently from bf0d6ac to 3b70ce2 Compare April 5, 2023 22:01
@matzipan
Copy link
Collaborator Author

matzipan commented May 10, 2023

@helgee, we might need to do a bit of a rewrite on the NAIF object stuff. I need a simple way to do the mapping between the ID and the correct object. What I have now looks like:

pub fn map_to_body(id: i32) -> Box<dyn NaifId> {
    let bla = match id {
        10i32 => sun::Sun,
        0i32 => barycenters::SolarSystemBarycenter,
        1i32 => barycenters::MercuryBarycenter,
         ...
    };


    return Box::new(bla);
}

But this is annoying, because now just to return the trait stuff we have to add a heap-allocated Box. Even then, this code doesn't compile because technically Sun and SolarSystemBarycenter are different types and Rust doesn't know how to deal with this, so I'd have to do something ugly like maybe:

pub fn map_to_body(id: i32) -> Box<dyn NaifId> {
    match id {
        10i32 => Box::new(sun::Sun),
        0i32 => Box::new(barycenters::SolarSystemBarycenter),
        1i32 => Box::new(barycenters::MercuryBarycenter),
         ...
    }
}

So I would see this move closer to an enum, like below, where everything is stack allocated and doesn't involve funny stuff.

pub enum Bodies {
    EarthBarycenter,
    Sun
}

impl Bodies {
    fn id(&self) -> i32 {
        match self {
            Bodies::EarthBarycenter => 3i32,
            Bodies::Sun => 10i32,
        }
    }

    fn get_body(id: i32) -> Bodies {
        match id {
            3i32 => Bodies::EarthBarycenter,
            10i32 => Bodies::Sun
        }
    }
}

pub fn test() {
    let body = Bodies::get_object(3);

    let id = body.id();
}

@matzipan
Copy link
Collaborator Author

A design decision is to keep the NAIF body IDs as part of the structure and not do the conversion to the trait object at this level. Furthermore, we want to keep using the bodies as trait objects because we need the flexibility of an open type.

Shall use the date types in core.

The API should be similar to the JPLEphemeris.jl. And the execution time of the state function should be 1 microsecond.

@matzipan matzipan force-pushed the implement_spk_parser branch 10 times, most recently from a3a942c to bca9799 Compare October 16, 2023 21:20
@matzipan matzipan force-pushed the implement_spk_parser branch 4 times, most recently from 247d476 to bd789c6 Compare November 5, 2023 23:08
@matzipan matzipan force-pushed the implement_spk_parser branch 3 times, most recently from 89c3b01 to 423d63b Compare November 7, 2023 23:06
@matzipan
Copy link
Collaborator Author

Closing in favor of #15

@matzipan matzipan closed this Nov 11, 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.

3 participants