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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default steps 2D plots are too visible #294

Closed
Juanlu001 opened this Issue Dec 20, 2017 · 10 comments

Comments

2 participants
@Juanlu001
Member

Juanlu001 commented Dec 20, 2017

馃悶 Problem

With the default number of points, the steps in the pericenter are too visible:

Before After
index index2

This came after the propagation changes in plotting and the different way to compute the data points.

馃幆 Goal

Make the plots smooth again.

馃挕 Possible solutions

  • Increase the default num_points
  • Other?

馃搵 Steps to solve the problem

  • Comment below about what you've started working on.
  • Add, commit, push your changes
  • Submit a pull request and add this in comments - Addresses #<put issue number here>
  • Ask for a review in comments section of pull request
  • Celebrate your contribution to this project 馃帀

@Juanlu001 Juanlu001 added this to the 0.9 milestone Dec 20, 2017

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Dec 20, 2017

Another side effect:

index4

This is a slightly different issue, namely that the pericenter is not always included in the propagation.

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Dec 20, 2017

Notice that, while fixing the previous one, one might step into #265.

@Juanlu001 Juanlu001 added the blocked label Dec 20, 2017

@nikita-astronaut

This comment has been minimized.

Contributor

nikita-astronaut commented Feb 20, 2018

Started working on this issue

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Feb 20, 2018

Thanks @nikita-astronaut, keep us posted! Notice that the propagation might fail and distract you, see the last comment in this issue.

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Feb 21, 2018

This is a good moment to say that just increasing the number of points is not going to be the preferred solution. In the report "The generalized Sundman transformation for propagation of high-eccentricity elliptical orbits" we can read:

Because of the large number of satellites, attention must be paid to the total time of computation.
For very eccentric orbits, satisfactory accuracy can be maintained with a larger step size near apogee
than is used at perigee

Take a look at this plot:

screenshot-2018-2-21 a605040 pdf

We are now in the situation of Figure 2.d), since we're uniformly sampling the true anomaly:

https://github.com/poliastro/poliastro/blob/24a9299f/src/poliastro/twobody/orbit.py#L295-L302

And before we introduced the sample method, we were doing almost the same thing:

https://github.com/anhiga/poliastro/blob/cd0a31ca3e7ffc99a3/src/poliastro/plotting.py#L169-L182

The most immediate fix for this issue, in my view, would be to use a uniform sample of the eccentric anomaly instead of the true anomaly. However, from the same report:

Merson (Ref. 13) introduced the idea of using the value n = 3 / 2 in the generalized Sundman transformation, with an intent not of regularization per se, but of maximizing computational efficiency; see also Ref. 14. He gave an analysis showing that this value of n equally distributes the integration error around a full orbit, even if the eccentricity is high.

I will therefore accept a pull request (with plots of the before/after situation) that use the eccentric anomaly as a first step, but implementing an s-integrator as described in the report would be a great next step (see #253).

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Feb 21, 2018

I found another book that might be useful: "Regularization in Orbital Mechanics: Theory and Practice" (https://doi.org/10.1515/9783110559125)

@nikita-astronaut

This comment has been minimized.

Contributor

nikita-astronaut commented Feb 21, 2018

I will now read the report and try to implement the s-integration.

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Feb 21, 2018

Great! Nevertheless this issue has been fixed in #320, so I'm closing. Let's continue the discussion in #253, or in the upcoming pull requests.

@Juanlu001 Juanlu001 closed this Feb 21, 2018

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Feb 21, 2018

Or in #265, for that matter. Notice that, when we have a working propagator, we can include the pericenter in all the samplings if we consider it appropriate, see #294 (comment).

@Juanlu001

This comment has been minimized.

Member

Juanlu001 commented Mar 1, 2018

Another nice before/after table:

Before After
before after

This is with the same number of points. Veeeeeeeeery nice :)

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