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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Examine solutions in Lambert notebook #348

Closed
Juanlu001 opened this issue Apr 20, 2018 · 7 comments

Comments

@Juanlu001
Copy link
Member

commented Apr 20, 2018

馃悶 Problem

With the current master version, I'm getting solutions in strange locations of the plot:
png image 1200 x 900 pixels

I am not sure this was always the case, so it's worth it to rerun the notebook and assess whether these results are correct.

馃搵 Steps to solve the problem

  • Comment below about what you've started working on.
  • Add, commit, push your changes
  • Submit a pull request and add this in comments - Addresses #<put issue number here>
  • Ask for a review in comments section of pull request
  • Celebrate your contribution to this project 馃帀
@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

This has been like that at least for two years https://docs.poliastro.space/en/v0.7.0/examples/Revisiting%20Lambert's%20problem%20in%20Python.html perhaps it was never quite right?

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

The plot is taken from the Izzo paper:

Screenshot_2019-06-06 1403 2705 pdf

https://arxiv.org/abs/1403.2705

@jorgepiloto

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I also saw that the labels Elliptic and Hyperbolic are switched, I was uploading this image but you were faster than me 馃槃

@jorgepiloto

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

The current state of this:

  1. After comparing the poliastro Izzo's algorithm against pykep it got exactly the same results, so algorithm implementation seems to be right.

  2. I focused my attention on the _compute_T_min private function and realized that wrong results were obtained only for those values of ll = -1. If we increase the y-axis limit to also see the solutions M=3 we can notice that red crosses corresponding to ll = -1 are the problematic points:

problematic_lambert

  1. This value of lambda is a problematic one as this comment states. Therefore, I will try to further inspect this code section 馃憤
@jorgepiloto

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Alright, so it seems that the initial condition x_i when ll = -1 tremendously affects the output of the function _compute_T_min. By forcing each initial condition x_i to each M, those red crosses matched the lines. See:

possible_lambert

However, this is not a solution, of course. A deep study on _tof_equation and _halley method is required to better see why the initial condition makes the solution to diverge if ll = -1 馃槙

@Juanlu001 Juanlu001 added the bug label Jun 22, 2019

@Juanlu001 Juanlu001 added this to the 0.13 milestone Jun 22, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

From the paper:

Values of 位^2 close to unity indicate a chord of zero length, a case which is indeed extremely interesting in interplanetary trajectory design as it is linked to the design of resonant transfers.

@Juanlu001

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

Encouraged by @jorgepiloto progress on this issue, I couldn't help but look at it... And I found the problem!

@@ -370,7 +378,8 @@ def _compute_T_min(ll, M, numiter, rtol):
             y = _compute_y(x_i, ll)
             T_i = _tof_equation(x_i, y, 0.0, ll, M)
             x_T_min = _halley(0.1, T_i, ll, rtol, numiter)
-            T_min = _tof_equation(x_T_min, y, 0.0, ll, M)
+            new_y = _compute_y(x_T_min, ll)
+            T_min = _tof_equation(x_T_min, new_y, 0.0, ll, M)
 
     return [x_T_min, T_min]

The value of y is being reused with an old value of x for the case ll != 1, M != 0. For some reason, this seems to affect most the ll == -1 case.

The reason why the value y is computed outside of the time of flight equation is to avoid a repetitive calculation when evaluating the Householder iteration scheme:

y = _compute_y(p0, ll)
fval = _tof_equation(p0, y, T0, ll, M)
T = fval + T0
fder = _tof_equation_p(p0, y, T, ll)
fder2 = _tof_equation_p2(p0, y, T, fder, ll)
fder3 = _tof_equation_p3(p0, y, T, fder, fder2, ll)

Which was introduced in b2d1fce in a buggy way, therefore breaking the algorithm.

perhaps it was never quite right?

I knew it was right at some point!

986b85d

https://github.com/poliastro/poliastro/blob/986b85d/examples/Revisiting%20Lambert's%20problem%20in%20Python.ipynb

Juanlu001 added a commit to Juanlu001/poliastro that referenced this issue Jun 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can鈥檛 perform that action at this time.