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 propagation to true_anomaly + a test #334

Merged
merged 5 commits into from Mar 4, 2018

Conversation

3 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Mar 3, 2018

No description provided.

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Mar 3, 2018

@Juanlu001 can you please help me understand why the test failed? What it is actually about?

@shreyasbapat

This comment has been minimized.

Copy link
Member

shreyasbapat commented Mar 3, 2018

PEP8 issues. Use autopep8 tool to fix these :)

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 3, 2018

Codecov Report

Merging #334 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   80.88%   80.92%   +0.03%     
==========================================
  Files          30       30              
  Lines        1465     1468       +3     
  Branches      114      115       +1     
==========================================
+ Hits         1185     1188       +3     
  Misses        250      250              
  Partials       30       30
Impacted Files Coverage Δ
src/poliastro/twobody/orbit.py 95.45% <100%> (+0.1%) ⬆️

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 53ebfc2...0c42fac. Read the comment docs.

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Mar 3, 2018

Strange: coverage test says I don't cover new function in orbit, but this test is present...

@shreyasbapat

This comment has been minimized.

Copy link
Member

shreyasbapat commented Mar 3, 2018

The name of the test function need to be started with "test_"

nikita-astronaut added some commits Mar 3, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 3, 2018

Thanks @shreyasbapat! The PEP8 failures were in the Travis CI build:

https://travis-ci.org/poliastro/poliastro/jobs/348605680#L842

However you went a bit too far, since we relax the line length limit:

[pycodestyle]

I'm adding more comments.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Apart from the comments stated above, what we really want is that Orbit.propagate can receive a true anomaly value, in the same way that .sample can. We also need tests to check that easy cases work, for instance propagating to pericenter (nu = 0) or apocenter (nu = 180 deg).

@@ -203,8 +218,10 @@ def test_sample_with_nu_value():

def test_hyperbolic_nu_value_check():
# A custom hyperbolic orbit
r = [1.197659243752796E+09, -4.443716685978071E+09, -1.747610548576734E+09] * u.km
v = [5.540549267188614E+00, -1.251544669134140E+01, -4.848892572767733E+00] * u.km / u.s
r = [1.197659243752796E+09, -4.443716685978071E+09, -

This comment has been minimized.

@Juanlu001

Juanlu001 Mar 3, 2018

Member

These lines do not need splitting

@@ -104,7 +104,8 @@ def from_classical(cls, attractor, a, ecc, inc, raan, argp, nu, epoch=J2000):
"""
if ecc == 1.0 * u.one:
raise ValueError("For parabolic orbits use Orbit.parabolic instead")
raise ValueError(

This comment has been minimized.

@Juanlu001

Juanlu001 Mar 3, 2018

Member

This line does not need splitting

@@ -375,7 +385,8 @@ def delegated_(self_):
delegated = property(delegated_)

else:
raise AttributeError("'{}' object has no attribute '{}'".format(self.__class__, item))
raise AttributeError(

This comment has been minimized.

@Juanlu001

Juanlu001 Mar 3, 2018

Member

This line does not need splitting

@@ -237,6 +238,15 @@ def __str__(self):
def __repr__(self):
return self.__str__()

def set_true_anomaly(self, target_nu):

This comment has been minimized.

@Juanlu001

Juanlu001 Mar 3, 2018

Member

I don't like this approach very much. On the one hand, the set_* seems to suggest that we are changing the value of an element in place, which we don't do. On the other hand, seems a bit heavy to do all the elements conversion to change only one value.

@shreyasbapat

This comment has been minimized.

Copy link
Member

shreyasbapat commented Mar 3, 2018

@Juanlu001 Welcome! But the autopep8 does not change the line lengths if used as per requirements. I always use it :)

@Juanlu001
Copy link
Member

Juanlu001 left a comment

I think this is good to go now. Thanks!

@Juanlu001 Juanlu001 merged commit 98f1e68 into poliastro:master Mar 4, 2018

5 checks passed

codeclimate All good!
Details
codecov/patch 100% of diff hit (target 80.88%)
Details
codecov/project 80.92% (+0.03%) compared to 53ebfc2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nikita-astronaut nikita-astronaut deleted the nikita-astronaut:iss329 branch Mar 4, 2018

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