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

(feat) breakdown Lambert::stardard() function #185

Merged
merged 4 commits into from
Jun 15, 2023
Merged

(feat) breakdown Lambert::stardard() function #185

merged 4 commits into from
Jun 15, 2023

Conversation

ThibFrgsGmz
Copy link
Contributor

To ease readability - too many lines so to save mental load of the reader.

@codecov
Copy link

codecov bot commented Jun 11, 2023

Codecov Report

Patch coverage: 59.25% and no project coverage change.

Comparison is base (c24f986) 66.69% compared to head (3443b22) 66.69%.

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     
Impacted Files Coverage Δ
src/tools/lambert.rs 76.11% <59.25%> (+1.49%) ⬆️

... and 4 files with indirect coverage changes

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

Copy link
Member

@ChristopherRabotin ChristopherRabotin left a 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.

/// # Returns
///
/// `(f64, f64, f64)` - The new values of phi, c2, and c3.
fn calculate_phi_and_c2_c3(phi: f64) -> (f64, f64, f64) {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups - done.

/// # Returns
///
/// `Result<f64, NyxError>` - The direction multiplier or an error if the transfer kind is not supported.
fn calculate_dm(
Copy link
Member

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ChristopherRabotin ChristopherRabotin merged commit 63b78f1 into nyx-space:master Jun 15, 2023
20 of 22 checks passed
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.

None yet

2 participants