Fix km -> m error and add type hints

[PabloVD]

• Tickets addressed: bsk-000
• Review: By commit
• Merge strategy: squash and merge

Description

• Fix incorrect units in docs as stated in issue Incorrect units in major axis in docs #1157
• Add some type hints in orbital elements for better intellisense in code editors.

Verification

Units verified by example codes.

Documentation

Documentation about orbital elements.

[schaubh]

I can see how this might be confusing, But, this is not strictly true. The distance units of 'a' and 'mu' must be the same. If both use km or m we are ok. Can we change this to discuss this?

[PabloVD]

While this is true for the specific function, the mu provided from the gravfactory are in m^3s^-2, and thus the general use in functions like orbitalMotion.elem2rv(mu, oe) would be in m, right?. Maybe it would be more clear for the user to have everything in meters there.
Also, if we want to set initial positions from the orbital elements with scObject.hub.r_CN_NInit = rN, they expect to be in m, right?

[schaubh]

Your statement is true for mu from gravFactory. Maybe then say in the units

m, needs to have the same distance unit as used in mu variable

That should clarify this, as well as have a better default unit?

[PabloVD]

I added a line like that in each docstring, do you think is more clear now?

[schaubh]

I'm ok with this approach. However, the CI tests are failing at the documentation stage. We are getting this warning:

/Users/runner/work/basilisk/basilisk/src/utilities/orbitalMotion.py:docstring of orbitalMotion.elem2rv_parab:12: WARNING: Inline emphasis start-string without end-string. [docutils]
looking for now-outdated files... none found
/Users/runner/work/basilisk/basilisk/src/utilities/orbitalMotion.py:docstring of orbitalMotion.rv2elem:16: WARNING: Inline emphasis start-string without end-string. [docutils]
/Users/runner/work/basilisk/basilisk/src/utilities/orbitalMotion.py:docstring of orbitalMotion.rv2elem_parab:16: WARNING: Inline emphasis start-string without end-string. [docutils]

Could you please test build the documentation and resolve this issue?

[PabloVD]

There was a problem with an asterisk in the docstring. The new commit should fix that.

[schaubh]

• made suggested edit to the unit statement
• need to rebase on latest develop

[schaubh]

Your statement is true for mu from gravFactory. Maybe then say in the units

m, needs to have the same distance unit as used in mu variable

That should clarify this, as well as have a better default unit?

[schaubh]

ok, I like how you clarified the unit issue. Can you please squash these commits now to two commits. One to update the units, one to add the method hints? This was we have a clean branch that does change prior commits.

[schaubh]

I'm ok with this approach. However, the CI tests are failing at the documentation stage. We are getting this warning:

/Users/runner/work/basilisk/basilisk/src/utilities/orbitalMotion.py:docstring of orbitalMotion.elem2rv_parab:12: WARNING: Inline emphasis start-string without end-string. [docutils]
looking for now-outdated files... none found
/Users/runner/work/basilisk/basilisk/src/utilities/orbitalMotion.py:docstring of orbitalMotion.rv2elem:16: WARNING: Inline emphasis start-string without end-string. [docutils]
/Users/runner/work/basilisk/basilisk/src/utilities/orbitalMotion.py:docstring of orbitalMotion.rv2elem_parab:16: WARNING: Inline emphasis start-string without end-string. [docutils]

Could you please test build the documentation and resolve this issue?

[PabloVD]

ok, I like how you clarified the unit issue. Can you please squash these commits now to two commits. One to update the units, one to add the method hints? This was we have a clean branch that does change prior commits.

The type hints were added in the same initial commit where I updated the units, so I cannot divide all commits in these two. Anyway, what about squash and merge the full PR in a single commit?

[schaubh]

There are ways to split a commit if needed. But here, I'm ok to have them be one commit that that helps you. However, we should not be having several incremental commits.

Also, instead of merging your branch with the latest develop, we ask contributors to do a rebase process instead.

[PabloVD]

Tried to rebase but I messed up something and now there are a bunch of commits in the commit history. We can remove them from the PR description when merged. Is it ok now?

[schaubh]

@PabloVD , agreed, this branch is a mess now. Not sure what happened here, but I can't accept this branch. Are you able to fix this? Or, I can cherry pick your PR related commits into a new branch and push from my side with a new PR?

[schaubh]

@PabloVD , are you still working on this branch to clean it up? Thanks for a quick update.

[schaubh]

@PabloVD , I'm going to close this PR request as there has been no development.
I created a new branch that provides the method variable hints for all the methods in orbitalMotion.py, and clarifies the units used for mu and a. This branch based on the latest develop, and I added release notes.
Thanks for supporting this fix to the BSK challenge you came across.