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.

Sign up for GitHub

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

Jump to bottom

Add orbit dynamics calculations #325

Merged
merged 30 commits into from
May 19, 2022
Merged

Add orbit dynamics calculations #325

merged 30 commits into from
May 19, 2022

Conversation

jianyangli
Copy link
Contributor

Add tisserand and d_criterion methods to Orbit class.

@pep8speaks
Copy link

pep8speaks commented Apr 27, 2022 •
edited
Loading

Hello @jianyangli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 764:80: E501 line too long (86 > 79 characters)
Line 831:80: E501 line too long (87 > 79 characters)
Line 833:80: E501 line too long (87 > 79 characters)

Line 204:13: E128 continuation line under-indented for visual indent
Line 210:13: E128 continuation line under-indented for visual indent

Comment last updated at 2022-05-19 21:47:17 UTC

@jianyangli jianyangli requested a review from mkelley April 27, 2022 03:53
@jianyangli jianyangli linked an issue Apr 27, 2022 that may be closed by this pull request
Orbital dynamics calcuations #294
Closed
@jianyangli jianyangli self-assigned this Apr 27, 2022
@jianyangli
Copy link
Contributor Author

I compared the numbers against references, but @mkelley can you check my implementation of the formula, especially for d_criterion, to make sure they are correct. Thanks.

@jianyangli jianyangli added this to the v0.4 milestone Apr 27, 2022
mkelley
mkelley reviewed May 5, 2022
View reviewed changes
sbpy/data/orbit.py Outdated Show resolved Hide resolved
sbpy/data/orbit.py Outdated Show resolved Hide resolved
sbpy/data/orbit.py Outdated Show resolved Hide resolved
sbpy/data/orbit.py Show resolved Hide resolved
sbpy/data/orbit.py Outdated Show resolved Hide resolved
sbpy/data/tests/test_orbit.py Outdated Show resolved Hide resolved
sbpy/data/tests/test_orbit.py Show resolved Hide resolved
mkelley
mkelley reviewed May 6, 2022
View reviewed changes
sbpy/data/orbit.py Outdated Show resolved Hide resolved
@Yeqzids
Copy link

Yeqzids commented May 6, 2022

Yes, it would be good to include different versions of $D$ if possible, since each has its own caveat and it is common to calculate/compare different $D$s. A complete summary of different versions of $D$ can be found in https://ui.adsabs.harvard.edu/abs/2019msme.book..210W/abstract, Section 9.2.1.

@jianyangli
Copy link
Contributor Author

Yes, it would be good to include different versions of $D$ if possible, since each has its own caveat and it is common to calculate/compare different $D$s. A complete summary of different versions of $D$ can be found in https://ui.adsabs.harvard.edu/abs/2019msme.book..210W/abstract, Section 9.2.1.

OK, Thanks @Yeqzids . I will add the other versions.

@jianyangli
Copy link
Contributor Author

@mkelley , I have a question for you. After implementing all three versions of D-criterion, I added all three references, Southworth & Hawkins 1963, Drummond 1981, and Jopek 1993 to bib track. But ideally, we only want to track the original reference for each corresponding version of D. E.g., if version='d', then only the Drummond 1981 reference should be tracked. Is there a way to do this? If not, what's approach would you suggest for this case?

@mkelley
Copy link
Member

mkelley commented May 9, 2022

Move the bib tracking calls to inside the method and place each within the their respective if-statement blocks.

@jianyangli
Copy link
Contributor Author

@mkelley Is this bib tracking implementation OK? Thanks.

@mkelley
Copy link
Member

mkelley commented May 13, 2022

Yes, I think this is right.

mkelley
mkelley reviewed May 13, 2022
View reviewed changes
docs/sbpy/data/orbit.rst Outdated Show resolved Hide resolved
mkelley
mkelley approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@mkelley mkelley left a comment

Choose a reason for hiding this comment

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

After those last two comments, I think this is good!

jianyangli added 11 commits May 16, 2022 11:20
@jianyangli
add tisserand method to Orbit
ed7f58b
@jianyangli
add d_criterion method to Orbit class
09b8b90
@jianyangli
small adjustments
0c1bd88
@jianyangli
bugfix in calculating d_criterion method
dff0537
@jianyangli
add tests for tisserand and d_criterion
857a9cf
@jianyangli
PEP8 fixes
68af008
@jianyangli
PEP8 fix
1c52205
@jianyangli
various improvements
a43901d 
* Change `.d_criterion` to `.D_criterion`
* Change reference for `D_criterion`, and add a reference section to docstring
* Improve examples
@jianyangli
revise tests
dd1a6c0 
* Add tests of D-criterion against Table 1 of Jopek 1993
* Add remote test for Tisserand parameter using the same epoch as JPL SBDB values
@jianyangli
PEP8 fix
65c3e2c
@jianyangli
correct an error in reference
b1b987c
@jianyangli jianyangli force-pushed the orbit_dynamics_cal branch from e3e22fd to 62e2d96 Compare May 16, 2022 15:20
@jianyangli
Copy link
Contributor Author

@mkelley I rebased this branch to merge, but then the read the docs build failed. I don't understand the error and can't find any problem with my local tests:

/home/docs/checkouts/readthedocs.org/user_builds/sbpy/envs/325/lib/python3.9/site-packages/sbpy/data/orbit.py:docstring of sbpy.data.orbit.Orbit.D_criterion:36: ERROR: Unexpected indentation.

Do you know what's the problem? Thanks.

@mkelley
Copy link
Member

mkelley commented May 16, 2022

I can't reproduce it either. Could it be the blank line on 818? Or the spaces before ")" on 817?

@jianyangli
Copy link
Contributor Author

jianyangli commented May 16, 2022 •
edited
Loading

Lines 815 - 817 are now identical to lines 757 - 759, but the latter passed with no problem. I also removed the blank line following these lines, no luck either.

Since this is not a required test, should we just go ahead and merge, and diagnose the problem later? I doubt it's a problem associated with the code.

@mkelley
Copy link
Member

mkelley commented May 19, 2022

@jianyangli Got it! I tested this out on a separate branch and found that the error is due to the indentation on lines 807 - 809. Try reformatting the 'References' section to avoid this indentation.

@mkelley
Copy link
Member

mkelley commented May 19, 2022

Oh, and please add a CHANGELOG entry for these new features.

@jianyangli jianyangli merged commit f01aec0 into NASA-Planetary-Science:master May 19, 2022
@jianyangli jianyangli deleted the orbit_dynamics_cal branch May 19, 2022 22:20
@jianyangli
Copy link
Contributor Author

Thank you @mkelley ! It took me some commits to fix all the bugs and citations in the docstrings, but glad it finally worked.

@mkelley
Copy link
Member

mkelley commented May 20, 2022

Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkelley mkelley mkelley approved these changes

Assignees

@jianyangli jianyangli

Labels
data new feature
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Orbital dynamics calcuations
4 participants
@jianyangli @pep8speaks @Yeqzids @mkelley