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

added solar drag #388

Merged
merged 7 commits into from Jun 15, 2018

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Jun 14, 2018

Seems like everything's working for now

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Overall the code looks good. I left some comments, but the summary is:

  • Simplify test code as much as possible
  • Avoid using specific bodies in the docstrings (Earth & Sun), since these functions are totally general and don't depend on an special configuration.
R_Earth=Earth.R.to(u.km).value, C_R=2.0, A=2e-4, m=100, Wdivc_s=Sun.Wdivc.value, sun=sun_normalized)

delta_as, delta_eccs, delta_incs, delta_raans, delta_argps, delta_hs = [], [], [], [], [], []
for ri, vi in zip(r, v):

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

Wow, so this is the required code to get a time history of orbital elements... There should be an easier way to do it. Not for this pull request, of course.

@@ -168,3 +169,57 @@ def test_3rd_body_Curtis(test_params):
(argp_f * u.rad).to(u.deg) - test_params['orbit'][4]],
[test_params['raan'], test_params['inc'], test_params['argp']],
rtol=1e-1)


solar_pressure_checks = [{'t_days': 200, 'deltas_expected': [3e-3, -8e-3, -0.035, -80.0]},

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

Can't we use pytest.mark.parametrize instead?

This comment has been minimized.

@nikita-astronaut

nikita-astronaut Jun 14, 2018

Author Contributor

No, we can't, because we first calculate trajectory and then check it on some special time values. If we calculate trajectory multiple times, testing would take tooooo long

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

OK, I understand. I think this could be achieved more cleanly using xunit setup functions https://docs.pytest.org/en/latest/xunit_setup.html or fixtures https://docs.pytest.org/en/latest/fixture.html#scope-sharing-a-fixture-instance-across-tests-in-a-class-module-or-session but let's not block the pull request because of this.

{'t_days': 1095, 'deltas_expected': [0.0, 0.06, -0.165, -10.0]},
]

drag_force_orbit = [10085.44 * u.km, 0.025422 * u.one, 88.3924 * u.deg,

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

Shouldn't this be defined inside the test function?

@@ -99,3 +99,65 @@ def third_body(t0, state, k, k_third, third_body):
body_r = third_body(t0)
delta_r = body_r - state[:3]
return k_third * delta_r / norm(delta_r) ** 3 - k_third * body_r / norm(body_r) ** 3


def shadow_function(r_sat, r_sun, R_Earth):

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

Does this apply only for the Earth? Or in fact any attractor around any star?

Parameters
----------
r_sat : numpy.ndarray
position of the satellite in the Earth frame (km)

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

Same as above: we should avoid the Earth/Sun terminology and use attractor/star or similar. This should work for satellites around Mars, or around planets that orbit stars other than the Sun.

return theta < theta_1 + theta_2


def solar_pressure(t0, state, k, R_Earth, C_R, A, m, Wdivc_s, sun):

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 14, 2018

Member

Rename to radiation_pressure?

r_sat = state[:3]
P_s = Wdivc_s / (norm(r_sun) ** 2)

nu = float(shadow_function(r_sat, r_sun, R_Earth))

This comment has been minimized.

@Juanlu001
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 14, 2018

Also, this needs a rebase on current master.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #388 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   83.44%   83.59%   +0.14%     
==========================================
  Files          34       34              
  Lines        1758     1774      +16     
  Branches      143      143              
==========================================
+ Hits         1467     1483      +16     
  Misses        252      252              
  Partials       39       39
Impacted Files Coverage Δ
src/poliastro/bodies.py 66.66% <100%> (+0.18%) ⬆️
src/poliastro/twobody/perturbations.py 100% <100%> (ø) ⬆️
src/poliastro/constants.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 505f453...96ba7e9. Read the comment docs.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

All tests passed now! But there's an extra import, and also this still needs a rebase on current master.

@@ -101,6 +102,7 @@ def third_body(t0, state, k, k_third, third_body):
return k_third * delta_r / norm(delta_r) ** 3 - k_third * body_r / norm(body_r) ** 3


@jit

This comment has been minimized.

@Juanlu001

Juanlu001 Jun 15, 2018

Member

Just as a clarification, norm is not jit-eable because it's meant to preserve units of quantities.

@@ -1,6 +1,7 @@
import pytest
import functools
import numpy as np
import cProfile

This comment has been minimized.

@Juanlu001

@Juanlu001 Juanlu001 merged commit b3f242d into poliastro:master Jun 15, 2018

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch 100% of diff hit (target 83.44%)
Details
codecov/project 83.59% (+0.14%) compared to 505f453
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 15, 2018

Merged! Thanks @nikita-astronaut 👏

@nikita-astronaut nikita-astronaut deleted the nikita-astronaut:solar_pressure branch Jun 15, 2018

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