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

changed default num_points in OrbitPlotter class #320

Merged
merged 3 commits into from Feb 21, 2018

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented Feb 20, 2018

Addresses #294
I simple changed the default num_points in OrbitPlotter class, now orbits in the example look smooth.
Still, this is not the best solution. Nonsmoothness appears in the vicinity of perihelion, cause the speed there is the highest, but the points in time are chosen uniformly, so the difference in position is the largest.

What I think could be done next is to place points non uniformly but with density ~1/current_speed, so that between every two points the position change is the same.

What do you think?

P.S. I am actually looking to joining your project later in the GSoC18.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Feb 21, 2018

Hi @nikita-astronaut, thanks for your interest in poliastro!

As you say, just increasing the number of points is not the best solution. I added more information in the original issue, so please take your time to read it.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 21, 2018

Codecov Report

Merging #320 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #320      +/-   ##
=========================================
+ Coverage   80.19%   80.2%   +0.01%     
=========================================
  Files          30      30              
  Lines        1439    1440       +1     
  Branches      113     113              
=========================================
+ Hits         1154    1155       +1     
  Misses        252     252              
  Partials       33      33
Impacted Files Coverage Δ
src/poliastro/plotting.py 76.43% <100%> (ø) ⬆️
src/poliastro/twobody/orbit.py 92.96% <100%> (+0.05%) ⬆️

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 24a9299...658f199. Read the comment docs.

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Feb 21, 2018

Hi @Juanlu001
I was surprised (but why?) that the functions of transitions between anomalies are already present in the package, so I used them. Now the angle is sampled uniformly from eccentric anomaly, not from the true anomaly. Seems like now the orbit looks smooth for only num_points = 150 instead of num_points = 300 I used previously.

before
after_1
after_2

I attached three images. The first is how it was before (true anomaly + 100 points), second: eccentric anomaly + 100 points (the problem is ameliorated but nonsmoothness however is still seen), third: eccentric anomaly + 150 points (orbit looks smooth).

Now, I think, I'll try to go further and work with s-integrator you've mentioned.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Thanks! Now this is a change we can include in poliastro I think. I left some comments and also would ask you to add a test or two for the new functionality.

@@ -29,7 +29,9 @@
{
"cell_type": "code",
"execution_count": 1,
"metadata": {},
"metadata": {

This comment has been minimized.

@Juanlu001

Juanlu001 Feb 21, 2018

Member

Can you restore the changes in this notebook?

"""Constructor.
Parameters
----------
ax : ~matplotlib.axes.Axes
Axes in which to plot. If not given, new ones will be created.
num_points : int, optional
Number of points to use in plots, default to 100.
Number of points to use in plots, default to 300.

This comment has been minimized.

@Juanlu001

Juanlu001 Feb 21, 2018

Member

Update docstring :)

@@ -4,10 +4,13 @@

from astropy import units as u
from astropy import time

from astropy.coordinates import CartesianRepresentation, get_body_barycentric_posvel

from poliastro.constants import J2000
from poliastro.twobody.angles import nu_to_M

This comment has been minimized.

@Juanlu001

Juanlu001 Feb 21, 2018

Member

What about importing all these functions in the same line?

@@ -294,12 +297,17 @@ def sample(self, values=None, function=propagate):

elif isinstance(values, int):
if self.ecc < 1:
nu_values = np.linspace(0, 2 * np.pi, values) * u.rad
# first sample eccenric anomaly, then transform into true anomaly

This comment has been minimized.

@Juanlu001

Juanlu001 Feb 21, 2018

Member

s/eccenric/eccentric/g

Also, what about adding a reference to the report in the comment? "Why sample uniformly on the eccentric anomaly to minimize error in the apocenter, see ..."

else:
# Select a sensible limiting value for non-closed orbits
# This corresponds to r = 3p
nu_limit = np.arccos(-(1 - 1 / 3.) / self.ecc)
nu_values = np.linspace(-nu_limit, nu_limit, values)
E_limit = nu_to_E(nu_limit, self.ecc) / u.rad

This comment has been minimized.

@Juanlu001

Juanlu001 Feb 21, 2018

Member

Not sure if the eccentric anomaly makes sense in the context of hyperbolic orbits. I wouldn't be surprised if this fails.

@nikita-astronaut

This comment has been minimized.

Copy link
Contributor Author

nikita-astronaut commented Feb 21, 2018

I think I fixed everything you were asking)

@Juanlu001 Juanlu001 merged commit 0492f60 into poliastro:master Feb 21, 2018

5 checks passed

codeclimate All good!
Details
codecov/patch 100% of diff hit (target 80.19%)
Details
codecov/project 80.2% (+0.01%) compared to 24a9299
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Feb 21, 2018

Awesome! Thanks for your contribution 😁

@nikita-astronaut nikita-astronaut deleted the nikita-astronaut:iss294 branch Feb 21, 2018

@Juanlu001 Juanlu001 referenced this pull request Aug 6, 2018

Merged

writing my GSoC'18 report #17

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