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:J2adding #341

Merged
merged 9 commits into from Apr 5, 2018

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Apr 3, 2018

No description provided.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 3, 2018

Codecov Report

Merging #341 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   81.26%   81.55%   +0.28%     
==========================================
  Files          30       31       +1     
  Lines        1473     1496      +23     
  Branches      115      115              
==========================================
+ Hits         1197     1220      +23     
  Misses        248      248              
  Partials       28       28
Impacted Files Coverage Δ
src/poliastro/bodies.py 66.12% <100%> (+0.37%) ⬆️
src/poliastro/twobody/propagation.py 95.55% <100%> (+0.37%) ⬆️
src/poliastro/constants.py 100% <100%> (ø) ⬆️
src/poliastro/twobody/perturbations.py 100% <100%> (ø)
src/poliastro/twobody/orbit.py 95.45% <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 7e04548...45eb037. Read the comment docs.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 3, 2018

This looks really promising :) However, I guess it's a work in progress since the J2 constant of the Earth is hardcoded there, instead of allowing for a different J2. If that's the case, please edit the PR title to start with WIP: .

Perhaps the most sensible place to put known J2 constants would be bodies.py. I hesitate whether to add an extra J2 parameter, or use the extra kwargs introduced in #235. Your call.

Also, I acknowledge that it will be difficult to find validation cases for bodies different than the Earth. That's why we need to validate against SPICE, STK or GMAT.

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Apr 3, 2018

@Juanlu001 , yes, indeed in progress) Please have a look at matrix. I am thinking on how to best change the signature for allowing the other J2

@nikita-astronaut nikita-astronaut changed the title J2adding WIP:J2adding Apr 3, 2018

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Apr 3, 2018

Is there a point to validate for the other bodies if the validation for Earth passes? Well, what type of mistake could have been remained unnoticed in this case?

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 3, 2018

In this case, the mistake is obvious: with the current implementation, if you try with Mars the validation would fail because the J2 value is hardcoded.

However, it's unlikely that you will find textbook examples with other bodies, or in general other values of J2. I added some discussion on the topic here:

https://github.com/poliastro/poliastro/wiki/Validation

This simple change is a good opportunity to try some of these validation approaches.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 3, 2018

@AunSiro - keep an eye on this pull request, I might need your help :)

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 5, 2018

Just to clarify a conversation @nikita-astronaut and I just had privately: the change in signature of the propagation methods only affects their performance if they're used directly. The method Orbit.propagate still needed unit conversion, and the same goes to Orbit.sample.

@Juanlu001 Juanlu001 removed their assignment Apr 5, 2018

@Juanlu001
Copy link
Member

Juanlu001 left a comment

I added mostly documentation comments and one concern about the API and the units, but this looks awesome!


r0 = np.array([1131.340, -2282.343, 6672.423]) # km
v0 = np.array([-5.64305, 4.30333, 2.42879]) # km/s
tof = 40 * 60.0 # s
orbit = Orbit.from_vectors(Earth, r0 * u.km, v0 * u.km / u.s)

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

Now these tests look nicer :)

@@ -237,7 +237,7 @@ def __str__(self):
def __repr__(self):
return self.__str__()

def propagate(self, value, method=mean_motion, rtol=1e-10):
def propagate(self, value, method=mean_motion, rtol=1e-10, **kwargs):

This comment has been minimized.

@Juanlu001


def J2_perturbation(t0, state, k, J2, R):
r"""Calculates J2_perturbation acceleration (km/s2)

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

You don't need the r unless you're using backslashes \ inside the docstring. Time to add the equation for the J2 perturbation? 😃

@@ -14,7 +14,7 @@
from poliastro.stumpff import c2, c3


def func_twobody(t0, u_, k, ad):
def func_twobody(t0, u_, k, ad, ad_kwargs):

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

Can we document these as well?

Initial position (km).
v0 : array
Initial velocity (km).
orbit : Orbit

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

Use ~poliastro.twobody.orbit.Orbit to have proper linking in the docs, like

orbit : ~poliastro.twobody.orbit.Orbit
(result in http://docs.poliastro.space/en/latest/api.html#poliastro.neos.dastcom5.orbit_from_record)

Initial velocity (km).
ad : function(t0, u, k), optional
Non Keplerian acceleration (km/s2), default to None.
orbit : Orbit

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

Same as above

Initial position (km).
v0 : array
Initial velocity (km).
orbit : Orbit

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

Same as above

Howard Curtis, (12.30)
"""

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

Extra whitespace


factor = (3.0 / 2.0) * k * J2 * (R.to(u.km).value ** 2) / (r ** 5)

a_x = 5.0 * r_vec[2] ** 2 / r ** 2 - 1

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

The equations look good to me.

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

factor = (3.0 / 2.0) * k * J2 * (R.to(u.km).value ** 2) / (r ** 5)

This comment has been minimized.

@Juanlu001

Juanlu001 Apr 5, 2018

Member

I think we should keep the perturbations units-agnostic, at least for now. The API would look something like this:

r, v = cowell(orbit, tof, ad=J2_perturbation, J2=Earth.J2.value, R=Earth.R.to(u.km).value)

I prefer to start with this (ugly) API and then improve when we figure out than having to regret adding it and go back because the performance is bad.

@Juanlu001 Juanlu001 merged commit 000eaac into poliastro:master Apr 5, 2018

4 of 5 checks passed

codeclimate Code Climate is analyzing this code.
Details
codecov/patch 100% of diff hit (target 81.26%)
Details
codecov/project 81.55% (+0.28%) compared to 7e04548
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 Apr 5, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Apr 5, 2018

Thanks for the addition @nikita-astronaut! This is the beginning of something big 🛰

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