Skip to content

from_state_vector: Fixed arg_pe incorrect when it should be > 180°.#45

Open
mgiuca wants to merge 1 commit into
RazerM:masterfrom
mgiuca:from_state_vector-arg_pe-gt-180
Open

from_state_vector: Fixed arg_pe incorrect when it should be > 180°.#45
mgiuca wants to merge 1 commit into
RazerM:masterfrom
mgiuca:from_state_vector-arg_pe-gt-180

Conversation

@mgiuca
Copy link
Copy Markdown
Contributor

@mgiuca mgiuca commented Sep 16, 2025

Adds a missing check for the case where the eccentricity vector is in the third or fourth quadrant, requiring arg_pe to be negated.

Note that this causes an edge case where arg_pe should be 0° but it comes out as 360° because we are not using modulo, but rather adding 2*pi. I didn't want to fix that in this commit because all the calculations in from_state_vector behave the same way.

Fixes #39.

Adds a missing check for the case where the eccentricity vector is in the third
or fourth quadrant, requiring arg_pe to be negated.

Note that this causes an edge case where arg_pe should be 0° but it comes out as
360° because we are not using modulo, but rather adding 2*pi. I didn't want to
fix that in this commit because all the calculations in from_state_vector behave
the same way.

Fixes RazerM#39.
@mgiuca
Copy link
Copy Markdown
Contributor Author

mgiuca commented Sep 16, 2025

Hi @RazerM , thanks for merging the test changes a few weeks ago. Now that that is in, I can fix the bugs I have been filing and be reasonably confident I'm not breaking anything (each of my fixes includes uncommenting some commented-out test cases that were broken before).

I am sending 6 PRs your way, which may be overwhelming but I want to ensure each bug is addressed individually since otherwise it would be too hard to reason about the correctness of each fix. No need to rush, I just send them all together in case you want to look at them all at once instead of drip-feeding them.

Thanks!

Matt

@mgiuca
Copy link
Copy Markdown
Contributor Author

mgiuca commented Oct 27, 2025

Hi @RazerM

Just wanted to check on this to make sure it isn't lost. (This PR and the other 5 I sent at the same time.)

Understanding this is a large volume of PRs. If it makes your life easier I can combine them into one, but I thought it would be better to split them so each one tackles a separate issue. Please let me know if I can help move this along.

Cheers

Matt

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.

from_state_vector gets arg_pe wrong if it's supposed to be > 180°

1 participant