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 atmospheric drag + tests #343

Closed
wants to merge 10 commits into
base: master
from

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Apr 10, 2018

Added atmospheric drag perturbation and tests. For links, see comments to the functions.

nikita-astronaut added some commits Apr 10, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 10, 2018

Codecov Report

Merging #343 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   81.55%   81.68%   +0.13%     
==========================================
  Files          31       31              
  Lines        1496     1507      +11     
  Branches      115      115              
==========================================
+ Hits         1220     1231      +11     
  Misses        248      248              
  Partials       28       28
Impacted Files Coverage Δ
src/poliastro/maneuver.py 95.83% <ø> (ø) ⬆️
src/poliastro/twobody/propagation.py 95.55% <ø> (ø) ⬆️
src/poliastro/constants.py 100% <100%> (ø) ⬆️
src/poliastro/twobody/perturbations.py 100% <100%> (ø) ⬆️
src/poliastro/bodies.py 66.48% <100%> (+0.36%) ⬆️

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 000eaac...c045e2a. Read the comment docs.

@Juanlu001 Juanlu001 requested review from Juanlu001 and AunSiro Apr 10, 2018

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Overall this looks good, but I asked for more comments, your opinion on a minor API change, and some more details about the approximation. I didn't do the pen-and-paper myself, just looks like a big leap.


# parameters of a circular orbit with h = 10 km
h = 250 # km
r0 = np.array([R + h, 0, 0]) # km

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

What about using Orbit.circular?

k : float
gravitational constant, (km^3/s^2)
B: float
ballistic coefficient B = C_D[] x A[m^2] / m[kg], (m^2 / kg)

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

Very good way to document the units

B: float
ballistic coefficient B = C_D[] x A[m^2] / m[kg], (m^2 / kg)
H0 : float
the e-times density decay heigth, (km)

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

Perhaps "atmospheric scale height" is more descriptive? (from utexas website)

C_D = 2.2 # dimentionless
A = ((np.pi / 4.0) * (u.m**2)).to(u.km**2).value # km^2
m = 100 # kg
B = C_D * A / m

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

What do you think as having C_D and m as input instead of B? So we compute B inside the function

v0 = np.array([0, np.sqrt(k / (R + h)), 0]) # km/s

# parameters of a body
C_D = 2.2 # dimentionless

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

Do these parameters matter for the test itself? Or any value would do?

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

If any value would do, let's clarify that in the test

orbit = Orbit.from_vectors(Earth, r0 * u.km, v0 * u.km / u.s)
tof = 100000 # s

dr_expected = -B * tof * rho0 * np.exp(-(norm(r0) - R) / H0) * np.sqrt(k * norm(r0))

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

This is related to the approximation trick you mention in the first comment I guess. Can we move that comment next to this line, and put * tof at the end?


def test_atmospheric_drag():
# http://farside.ph.utexas.edu/teaching/celestial/Celestialhtml/node94.html#sair (10.148)
# given the expression for \dot{a} / a, aproximate \Delta a \approx F_a * \Delta t

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 12, 2018

Member

Can you elaborate on this approximation?

nikita-astronaut added some commits Apr 12, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 13, 2018

The math checks out :) (leaving this here for future reference, as it was not obvious for me)

photo5947563226105490423

Going to merge this manually.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 13, 2018

Merged in 35a2c34. Thanks @nikita-astronaut! 🚀

@Juanlu001 Juanlu001 closed this Apr 13, 2018

@nikita-astronaut nikita-astronaut deleted the nikita-astronaut:atmospheric_drag branch Apr 14, 2018

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