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

WIP: working on thrust #400

Merged
merged 16 commits into from Jul 13, 2018

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Jul 6, 2018

Hello! I've made in through your .pdf file) I started to move thrusts to poliastro. Here is one perturbation, test pass, please have a look, how do you find it?

Any suggestions and thoughts are welcomed) Moving the other thrusts is not difficult, your code is great) Though it is nice to see that poliastro API changed a bit since you wrote that code)

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jul 6, 2018

Hah! Tests are now failing because of #397. Temporarily, you can set plotly<3.0 in setup.py while we figure out #399.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jul 6, 2018

(By the way, all of Travis CI builds timed out but you can see the failing tests in AppVeyor)

@Juanlu001
Copy link
Member

Juanlu001 left a comment

I proposed a different organization for the code.

Transfer Problem Using Optimal Control Theory", 1997.
"""

def _beta_0(V_0, V_f, inc_0, inc_f):

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 6, 2018

Member

I would move all these functions outside of the function edelbaum_ai. Perhaps we should have a package, and modules inside of it?

V_0, beta_0_, _ = _compute_parameters(k, a_0, a_f, inc_0, inc_f)

@state_from_vector
def a_d(t0, ss):

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 6, 2018

Member

Except of this function, which does depend of external parameters (and is essentially a closure).

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Jul 7, 2018

I've changed the code structure, is this how you meant it to be?)

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Jul 8, 2018

The test_leo_geo_numerical test takes too long. Maybe it is time to numba-jit our cowell method? I am ready.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Great!

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Jul 8, 2018

I can continue adding other thrusts, but we need to figure out how to speed-up tests...

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jul 8, 2018

To JIT cowell you would first need to JIT func_twobody, which would require you to pass a jitted perturbation acceleration. For this particular case, you would have to vectorize change_a_inc, which finally requires you to JIT beta. Now, going upwards again:

def func_twobody(...):
    ax, ay, az = ad(...)
    return _func_twobody(..., ax, ay, az)

@njit
def _func_twobody(...):
    ...

And this might or might not save some seconds.

I think this would be very valuable (and alleviate some of the pain we have with the tests). If you want to try these optimizations, I propose that you write some benchmark (a simple one using cowell and an easy perturbation) and try to explore jitclass to see if we can save some seconds. We can leave this PR on hold in the meanwhile: I will move the CI to Circle CI very soon and that might also be helpful.

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Jul 8, 2018

Thanks! I will finally read something about jitting)

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jul 9, 2018

Unless I'm doing something wrong, I think we've hit another numba issue:

numba/numba#3090

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jul 10, 2018

This needs a rebase on current master.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 10, 2018

Codecov Report

Merging #400 into master will increase coverage by 1.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
+ Coverage   84.15%   85.22%   +1.07%     
==========================================
  Files          34       39       +5     
  Lines        1792     1922     +130     
  Branches      141      143       +2     
==========================================
+ Hits         1508     1638     +130     
  Misses        248      248              
  Partials       36       36
Impacted Files Coverage Δ
src/poliastro/twobody/thrust/change_a_inc.py 100% <100%> (ø)
src/poliastro/twobody/thrust/change_inc_ecc.py 100% <100%> (ø)
src/poliastro/twobody/thrust/__init__.py 100% <100%> (ø)
src/poliastro/twobody/perturbations.py 100% <100%> (ø) ⬆️
src/poliastro/twobody/thrust/change_argp.py 100% <100%> (ø)
src/poliastro/util.py 100% <100%> (ø) ⬆️
...oliastro/twobody/thrust/change_ecc_quasioptimal.py 100% <100%> (ø)
... and 2 more

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 a33c404...382e86c. Read the comment docs.

@nikita-astronaut nikita-astronaut force-pushed the nikita-astronaut:adding_maneuver branch from c8dfa79 to f02502f Jul 12, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jul 12, 2018

See #313, I don't know why all the packages to read data from HORIZONS stopped working...

Let's ignore that issue for a moment. What are your plans for this pull request? If you want it in poliastro 0.10, this should be reviewed today and merged tomorrow, so we can release a beta.

nikita-astronaut added some commits Jul 12, 2018

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Mostly style comments! On the right path 👍

a_d, _, t_f = change_ecc_quasioptimal(s0, ecc_f, f)

# Propagate orbit
r, v = cowell(s0, tof=t_f, ad=a_d, rtol=1e-8)

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

I guess we now can do s0.propagate(t_f, method=cowell, ad=a_d) or similar? The API has evolved a lot during the last year :)

# Propagate orbit
r, v = cowell(s0, tof=t_f, ad=a_d, rtol=1e-8)

sf = Orbit.from_vectors(Earth,

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Same as above.

# Propagate orbit
r, v = cowell(s0, t_f, ad=argp_accel, rtol=1e-8)

sf = Orbit.from_vectors(Earth,

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Same as above.


r, v = cowell(s0, t_f, ad=edelbaum_accel, rtol=1e-7)

sf = Orbit.from_vectors(Earth, r * u.km, v * u.km / u.s, s0.epoch + t_f * u.s)

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Same as above.

"""
return np.sqrt(vec.dot(vec))


@jit
def norm_3d(vec):

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Let's be more consistent with how we @jit methods here:

def circular_velocity(k, a):
    ...

circular_velocity_fast = jit(circular_velocity)

def norm(vec):
    ...

norm_fast = jit(norm)

Notice though that this will require numba>=0.39. Also, you might have to do

@jit
def norm_fast(vec):
    return np.sqrt((1.0 * vec).dot(1.0 * vec))

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

So whenever we jit a function we should use norm_fast inside, and whenever we want to retain astropy units we should use norm. There's no alternative.


def change_argp(k, a, ecc, argp_0, argp_f, f):
"""Guidance law from the model.
Thrust is aligned with an inertially fixed direction perpendicular to the

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Whitespace, and also before Parameters

@@ -0,0 +1,62 @@
"""Quasi optimal eccentricity-only change, with formulas developed by Pollard.
References

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Whitespace


def change_ecc_quasioptimal(ss_0, ecc_f, f):
"""Guidance law from the model.
Thrust is aligned with an inertially fixed direction perpendicular to the

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Whitespace, and also before Parameters

@@ -0,0 +1,89 @@
"""Simultaneous eccentricity and inclination changes.
References

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Whitespace


def change_inc_ecc(ss_0, ecc_f, inc_f, f):
"""Guidance law from the model.
Thrust is aligned with an inertially fixed direction perpendicular to the

This comment has been minimized.

@Juanlu001

Juanlu001 Jul 12, 2018

Member

Whitespace, and also before Parameters

nikita-astronaut added some commits Jul 12, 2018

@Juanlu001 Juanlu001 merged commit 19ed5ff into poliastro:master Jul 13, 2018

10 checks passed

ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: quality Your tests passed on CircleCI!
Details
ci/circleci: test_py35 Your tests passed on CircleCI!
Details
ci/circleci: test_py36 Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch 100% of diff hit (target 84.15%)
Details
codecov/project 85.22% (+1.07%) compared to a33c404
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the 2 - In Progress label Jul 13, 2018

This was referenced Jul 13, 2018

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