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

Flatten branching loop in b_plane calc #275

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cmleinz
Copy link
Contributor

@cmleinz cmleinz commented Jan 26, 2024

Effects

This is mostly a stylistic change, feel free to disregard. Helps unify similar code across two branches

If this is a new feature or a bug fix ...

  • Yes, the branch I'm proposing to merge is called issue-xyz where xyz is the number of the issue.

If this change adds or modifies a validation case

  • No.
  • Yes and:
    • I have coded up, or updated, an existing test case; and
    • I have provided a GMAT script which confirms the result(s); and
    • I have updated the VALIDATION.md file with the new error data between nyx and GMAT.
    • I will specify the version of GMAT used for the validation.

@cmleinz
Copy link
Contributor Author

cmleinz commented Jan 26, 2024

Sorry for working GitHub actions unnecessarily! I'll look this over in the morning to see what invariants I'm violating.

return Ok((total_dv, b_plane));
let (norm, dv) = match ltof_err {
None => {
let v = Vector2::new(br_err, bt_err);
Copy link
Member

Choose a reason for hiding this comment

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

Oh, good find! The B-Plane Jacobian is indeed BR, BT, LTOF. That said, I think this should be fixed in the Jacobian() function so it returns BT, BR, LTOF as does the Jacobian2 function and as the documentation says.

https://rustdoc.nyxspace.com/src/nyx_space/cosmic/bplane.rs.html#140-152

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