Add lox-orbits crate
#89
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
- Coverage 98.13% 96.01% -2.13%
==========================================
Files 55 67 +12
Lines 19560 20792 +1232
==========================================
+ Hits 19195 19963 +768
- Misses 365 829 +464 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Lots of comments as it's a big PR, but they should all be quite straightforward.
I'll reiterate my usual plea for much more documentation. In the near term, not being able to read what each step of an algorithm is intended to do leaves me unable to sanity check it effectively. The Brent implementation in particular is a hurricane of arithmetic on cryptically named variables.
| let const_ident = format_ident!("{}", ident.to_string().to_uppercase()); | ||
|
|
||
| let output = quote! { | ||
| const #const_ident: #ident = #ident; |
There was a problem hiding this comment.
The singleton function needs a doc comment detailing the expected inputs and output, asince this output is quite puzzling to anyone who hasn't been part of our conversations. The derive macro is only applicable to the specific case of a zero-sized marker type whose single instance is representable as a constant.
| pub fn hyperbolic_to_true(hyperbolic_anomaly: f64, eccentricity: f64) -> f64 { | ||
| use lox_utils::types::units::Radians; | ||
|
|
||
| // Hyperbolic Anomaly <-> True Anomaly |
There was a problem hiding this comment.
This information would be better off as doc comments on the corresponding functions.
| @@ -18,6 +19,8 @@ pub trait BaseTwoBody { | |||
| fn to_keplerian_state(&self, grav_param: f64) -> BaseKeplerian; | |||
| } | |||
|
|
|||
| pub type BaseState = (BaseTime, BaseCartesian); | |||
There was a problem hiding this comment.
Blah, blah, doc comments for exported types, blah
| if n < 2 { | ||
| return events; | ||
| } | ||
| let time1 = time0 + *deltas.last().unwrap(); |
There was a problem hiding this comment.
It's probably worth implementing Add<&TimeDelta> for ergonomics. The implementation could just deref the delta and call the existing Add<TimeDelta> method.
| return events; | ||
| } | ||
|
|
||
| if signs.iter().all(|&s| s < 0.0) { |
There was a problem hiding this comment.
Comments throughout explaining why each step of the algorithm is doing what it's doing would be a huge help to those who come after. Porting the uncommented AstroTime.jl code is hard.
| } | ||
| } | ||
|
|
||
| fn perifocal(&self) -> (DVec3, DVec3) { |
There was a problem hiding this comment.
Is this (pos, vel) tuple type worth a struct declaration?
| fn sub(self, rhs: BaseTime) -> Self::Output { | ||
| let mut subsec = self.subsecond.0 - rhs.subsecond.0; | ||
| let mut seconds = self.seconds - rhs.seconds; | ||
| if subsec.is_sign_negative() { |
There was a problem hiding this comment.
I think this implementation is missing the case of (-lhs) - (-rhs) where the sign of the result becomes positive. The subsecond subtraction has to be treated as an addition, since we always represent subseconds as positive. These annoying branches are why, for TimeDelta Sub, I chose to use Neg on the rhs when it was negative and treat it as an addition.
| fn find_root(&self, f: F, initial_guess: f64) -> Result<f64, RootError> { | ||
| let mut p0 = initial_guess; | ||
| let mut i = 0u32; | ||
| while i < self.max_iter { |
There was a problem hiding this comment.
for _ in 0..self.max_iter or (0..self.max_iter).for_each should work here, since the only non-incrementing branch terminates the loop.
| fn find_root(&self, f: F, deriv: D, initial_guess: f64) -> Result<f64, RootError> { | ||
| let mut p0 = initial_guess; | ||
| let mut i = 0u32; | ||
| while i < self.max_iter { |
There was a problem hiding this comment.
Same comment about idiomatic loop construction.
| return Ok(xcur); | ||
| } | ||
|
|
||
| while i < self.max_iter { |
There was a problem hiding this comment.
Same comment about idiomatic loop construction.
WIP
Supersedes: #82 #83 #57