-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
(feat) breakdown Lambert::stardard() function #185
(feat) breakdown Lambert::stardard() function #185
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #185 +/- ##
=======================================
Coverage 66.69% 66.69%
=======================================
Files 127 127
Lines 32623 32623
=======================================
+ Hits 21758 21759 +1
+ Misses 10865 10864 -1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current Lambert solver is basic and breaks down for multi-revolution. It would probably be quite useful to have a better Lambert solver, especially for interplanetary mission design and contour plots.
Lamberthub has an explanation of these algorithms, and is released under the Apache 2.0 license here.
If you're interested, I'd love to review an implementation of the Izzo 2015 Lambert solver.
src/tools/lambert.rs
Outdated
/// # Returns | ||
/// | ||
/// `(f64, f64, f64)` - The new values of phi, c2, and c3. | ||
fn calculate_phi_and_c2_c3(phi: f64) -> (f64, f64, f64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be its own function: part of the Secant method is updating these values, and someone familiar with the secant method would expect to find this at the bottom of the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is still in the code I think, although the functionality has been restored to the standard
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oups - done.
src/tools/lambert.rs
Outdated
/// # Returns | ||
/// | ||
/// `Result<f64, NyxError>` - The direction multiplier or an error if the transfer kind is not supported. | ||
fn calculate_dm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be in the impl
of TransferKind
, something like this:
impl TransferKind {
fn direction_of_motion(self, r_final: &Vector3<f64>, r_init: &Vector3<f64>) -> Result<f64, NyxError> {
// Do the same math
}
}
Then you would call it directly like so: kind.direction_of_motion(&r_final, &r_init)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
To ease readability - too many lines so to save mental load of the reader.