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

J3 adding #398

Merged
merged 4 commits into from Jul 5, 2018
Merged

J3 adding #398

merged 4 commits into from Jul 5, 2018

Conversation

@nikita-astronaut
Copy link
Contributor

@nikita-astronaut nikita-astronaut commented Jul 4, 2018

Does not agree with the paper yet...

r_vec = state[:3]
r = norm(r_vec)

factor = (1.0 / 2.0) * k * J3 * (R ** 3) / (r ** 5)
Copy link
Member

@astrojuanlu astrojuanlu Jul 4, 2018

Choose a reason for hiding this comment

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

The formulas look right:

screenshot from 2018-07-04 14-21-59

Loading

Copy link
Contributor Author

@nikita-astronaut nikita-astronaut Jul 4, 2018

Choose a reason for hiding this comment

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

Yes, and I took derivative myself, 12.8 formula is right as well

Loading

@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Jul 4, 2018

The CI hasn't finished yet, but we can already see that the test failed. @nikita-astronaut confirmed privately that, while the semimajor axis does not match, eccentricity and inclination do. This is what we could do:

  • Derive the formulas given in Curtis, just in case there is some mistake there. (Done)
  • Look for another source of validated data.

We should try these just in case.

In the meanwhile, there are another two pressing issues: first, the tests take way too long to run (see #395) and second, the API for retrieving a time history of Keplerian elements is just horrible and error prone (could be fixed by #257, but maybe not).

We should consider stopping this work until we have solutions for them, but @nikita-astronaut if you don't feel comfortable with the latter (I would take care of the former anyway) let's make a shortcut to get this merged in an "experimental" state:

  • Add two tests: one that checks for the variables that do match, and another one marked as xfail that checks for the semimajor axis.
  • Add a warning on the J3 perturbation that points to this pull request and tells the user that this code is not yet fully validated.
  • Cleanup the debugging code and the commented lines.
  • Review & Merge.

What do you think?

Loading

@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Jul 4, 2018

Just saw that you already derived the formulas yourself: updated my comment.

Loading

@nikita-astronaut
Copy link
Contributor Author

@nikita-astronaut nikita-astronaut commented Jul 4, 2018

@Juanlu001 , looking for another source of validation is a good idea, I believe. I first will search for some more data, and use shortcut in case I don't find anything

Loading

@codecov
Copy link

@codecov codecov bot commented Jul 4, 2018

Codecov Report

Merging #398 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   84.03%   84.15%   +0.11%     
==========================================
  Files          34       34              
  Lines        1779     1792      +13     
  Branches      141      141              
==========================================
+ Hits         1495     1508      +13     
  Misses        248      248              
  Partials       36       36
Impacted Files Coverage Δ
src/poliastro/constants.py 100% <100%> (ø) ⬆️
src/poliastro/bodies.py 66.84% <100%> (+0.17%) ⬆️
src/poliastro/twobody/perturbations.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c4b08...60b53ae. Read the comment docs.

Loading

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Well done!

Loading

@poliastro poliastro deleted a comment from nikita-astronaut Jul 5, 2018
@astrojuanlu astrojuanlu merged commit 70f20d2 into poliastro:master Jul 5, 2018
6 checks passed
Loading
@nikita-astronaut nikita-astronaut deleted the J3/J4 branch Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants