This PR corrects an error in xy2tp that was revealed today (2020-08-28) during Petal0 tests. I also added specific error cases from the Petal0 testing today to the unit test. With this two line fix, those cases pass now.
DETAILS:
The fix is very simple, but the sequence of events when the bug occurs takes many words to describe!
When operating in theta_guess mode, xy2tp generates possible alternates for (t,p) in three steps:
a. Law of cosines calc --> (t,p) such that p within [0, 180]
b. Reflect coords from (c) across a line from origin through (x,y)
c. For each of these options, generate two more options, wrapping theta by +/-360 deg
Subsequently the best option (nearest to t_guess without exceeding range limits or t_guess_tol) is selected.
At each step of a, b, c, the proposed (t,p) is checked against range limits. In cases where the limits would be exceeded, the limit value itself is used, and a boolean is generated stating "range_fail".
The bug occurs when in some cases when we have a range_fail at step (a) or (b). In (c) additional (t,p) options would be generated based on these truncated values. If this occurs at the same time that theta is > 180 or < -180, and is opposite from t_guess --- then we have a loophole --- some new (c) option, might unwrap into a plausible range. However it would be starting its unwrap from a truncated value, and thus end up in the wrong place.
It's quite complicated to describe this chain of events --- and was tricky to find during debugging today! But in fact the solution is very simple 2 lines of code, which just check for a range_fail in (a) or (b), and if so don't try any secondary wrapped options with that (t,p) as a basis.
This PR corrects an error in xy2tp that was revealed today (2020-08-28) during Petal0 tests. I also added specific error cases from the Petal0 testing today to the unit test. With this two line fix, those cases pass now.
DETAILS: The fix is very simple, but the sequence of events when the bug occurs takes many words to describe!
When operating in theta_guess mode, xy2tp generates possible alternates for (t,p) in three steps:
a. Law of cosines calc --> (t,p) such that p within [0, 180] b. Reflect coords from (c) across a line from origin through (x,y) c. For each of these options, generate two more options, wrapping theta by +/-360 deg
Subsequently the best option (nearest to t_guess without exceeding range limits or t_guess_tol) is selected.
At each step of a, b, c, the proposed (t,p) is checked against range limits. In cases where the limits would be exceeded, the limit value itself is used, and a boolean is generated stating "range_fail".
The bug occurs when in some cases when we have a range_fail at step (a) or (b). In (c) additional (t,p) options would be generated based on these truncated values. If this occurs at the same time that theta is > 180 or < -180, and is opposite from t_guess --- then we have a loophole --- some new (c) option, might unwrap into a plausible range. However it would be starting its unwrap from a truncated value, and thus end up in the wrong place.
It's quite complicated to describe this chain of events --- and was tricky to find during debugging today! But in fact the solution is very simple 2 lines of code, which just check for a range_fail in (a) or (b), and if so don't try any secondary wrapped options with that (t,p) as a basis.