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

documenting new features #428

Merged
merged 13 commits into from Aug 3, 2018

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Jul 31, 2018

No description provided.

nikita-astronaut added some commits Jul 31, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 31, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage    85.2%   85.24%   +0.03%     
==========================================
  Files          47       47              
  Lines        2008     2013       +5     
  Branches      154      158       +4     
==========================================
+ Hits         1711     1716       +5     
  Misses        259      259              
  Partials       38       38
Impacted Files Coverage Δ
src/poliastro/twobody/propagation.py 96.66% <100%> (+0.51%) ⬆️
src/poliastro/core/_jit.py 100% <100%> (ø) ⬆️
src/poliastro/util.py 100% <100%> (ø) ⬆️
src/poliastro/ephem.py 100% <100%> (ø) ⬆️
src/poliastro/twobody/orbit.py 95.71% <100%> (-0.15%) ⬇️

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 335a37c...199d17d. Read the comment docs.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 1, 2018

It seems that you added a couple of things that were already there. Could you explain what's going on here? Also, I will make some comments.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Left a long comment about an important implementation detail.

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

return propagate(self, time_of_flight, method=method, rtol=rtol, **kwargs)

def sample(self, values=None, method=mean_motion):
def sample(self, values=None, method=mean_motion, dense_output=False):

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 1, 2018

Member

Instead of exposing dense_output to the user, I propose doing something else, since this is a very low level detail and can lead to confusion. For instance, they could do

sample(..., method=kepler, dense_output=True)

and it's not clear what should happen in this case. We can either:

  • Make dense_output=True/False a property of the propagator. The simplest, although a bit ugly way, would be doing something like
def cowell(...):
    ...

cowell.dense_output = True
kepler.dense_output = False

we could also use some dictionary somewhere:

DENSE_OUTPUT_METHODS = [cowell]

...
if method in DENSE_OUTPUT_METHODS:
    ...

or turn the propagators into classes:

class Cowell(Propagator):
    dense_output = True
    def __call__(self, ...):
        ...

...
orbit.sample(..., method=Cowell())

but I find this last option the least appealing if we're going this way.

  • Move the propagation loop to the propagator code, instead of having it in Orbit._sample. This could also be done in several ways: in particular, cowell already accepts a list of values in tof (something that the other propagators don't) so perhaps we should generalize this behavior and, if kepler gets a list of tof, do the for loop inside.

nikita-astronaut added some commits Aug 1, 2018

??
@Juanlu001

This comment has been minimized.

Copy link

Juanlu001 commented on f54a8c3 Aug 2, 2018

The commit message is not very descriptive, isn't it? 😉

@nikita-astronaut nikita-astronaut force-pushed the nikita-astronaut:documenting_new_features branch from 740bc50 to 80e1407 Aug 2, 2018

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Stylistic suggestions to unify some APIs, mostly.

Also, if we could plot the evolution of RAAN (Omega) with J2, it would be great. It's the basic problem everybody wants to do.

Other than that, impressive plots!

@@ -0,0 +1,33 @@
"""Just-in-time compiler.

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

This file should go away, as it now lives in src/poliastro/core/_jit.py

@@ -0,0 +1,204 @@
from astropy.time import Time

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

These functions now live in src/poliastro/core/perturbations.py


def kepler(orbit, tof, *, numiter=350, **kwargs):

def kepler(orbit, tofs, **kwargs):

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

Why the signature change here?


def kepler(orbit, tofs, **kwargs):
if not hasattr(tofs, '__len__'):
return _kepler(orbit, tofs)

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

It seems that _kepler is not receiving the **kwargs

@@ -15,3 +15,4 @@ Jupyter notebooks
/examples/Studying Hohmann transfers.ipynb
/examples/Using NEOS package.ipynb
/examples/Visualizing the SpaceX Tesla Roadster trip to Mars.ipynb
/examples/Natural and artificial perturbations.ipynb

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

I think there are also missing references to thrust in docs/source/core.rst

"j_date = 2454283.0\n",
"tof = (moon_geo['tof'] * u.day).to(u.s).value\n",
"# create interpolant of 3rd body coordinates (calling in on every iteration will be just too slow)\n",
"body_r = build_ephem_interpolant(body, moon_geo['period'], (j_date, j_date + moon_geo['tof']), rtol=1e-2)\n",

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

Now that I see it, build_ephem_interpolant should at least receive quantities with units, or leverage poliastro.util.time_range.

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

I mean, the user should not have to do j_date = 2454283.0.

"epoch = Time(j_date, format='jd', scale='tdb')\n",
"initial = Orbit.from_classical(Earth, *moon_geo['orbit'], epoch=epoch)\n",
"\n",
"cowell_with_3rdbody = functools.partial(cowell, rtol=1e-6, ad=third_body,\n",

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

Great!

38668.102674787464,
30789.277815565652,
18702.881438068034,
4175.49

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

Can we add the Moon here? 😄

38668.102674787464,
30789.277815565652,
18702.881438068034,
4175.49

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

time_range again

38668.102674787464,
30789.277815565652,
18702.881438068034,
4175.49

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

This is a bit low level, but it's a good thing - it's a sign that a higher level API should live as a Maneuver!

nikita-astronaut added some commits Aug 2, 2018

@@ -6,6 +6,7 @@
"""
import warnings
import inspect
import astropy.units as u

This comment has been minimized.

@Juanlu001

Juanlu001 Aug 2, 2018

Member

This is not needed, right?

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 2, 2018

The tests failed because of a naming issue in the notebook

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 3, 2018

Wow, something happened and some tests are not running:

https://circleci.com/gh/poliastro/poliastro/385

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 3, 2018

Confirmed:

$ pytest -vvv
============================================= test session starts ==============================================
platform linux -- Python 3.6.5, pytest-3.6.3, py-1.5.4, pluggy-0.6.0 -- /home/juanlu/.miniconda36/envs/poliastro36/bin/python
cachedir: .pytest_cache
rootdir: /home/juanlu/Development/poliastro/poliastro-library, inifile: setup.cfg
plugins: cov-2.5.1
collected 421 items
[...]
$ pip install -U pytest
[...]
$ pytest -vvv
============================================= test session starts ==============================================
platform linux -- Python 3.6.5, pytest-3.7.1, py-1.5.4, pluggy-0.7.1 -- /home/juanlu/.miniconda36/envs/poliastro36/bin/python
cachedir: .pytest_cache
rootdir: /home/juanlu/Development/poliastro/poliastro-library, inifile: setup.cfg
plugins: cov-2.5.1
collected 79 items                                                  

@Juanlu001 Juanlu001 merged commit 283036c into poliastro:master Aug 3, 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 85.2%)
Details
codecov/project 85.24% (+0.03%) compared to 335a37c
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 Aug 3, 2018

@nikita-astronaut nikita-astronaut deleted the nikita-astronaut:documenting_new_features branch Aug 4, 2018

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