Return times in Orbit.sample

[astrojuanlu]

🐞 Problem

Sampling an orbit was introduced in #263, and it just returns a CartesianRepresentation object:

https://github.com/poliastro/poliastro/blob/v0.8.0/src/poliastro/twobody/orbit.py#L315

It would be nice to return the time values as well.

🎯 Goal

Just sampling the positions is enough for things like plotting, but any application that needs to analyze the results would benefit from having the times as well.

💡 Possible solutions

• Return a tuple times, positions
• Return a new type of object (Trajectory?)

Notice that both of these break the API. Any solution that returns different things depending on an optional parameter won't be accepted.

📋 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 🎉

[ritiek]

I don't know much about poliastro's code base but another solution could be creating a new method (say get_times()) on an Orbit object that only returns times? Something like this:

    class Orbit(object):
    ...
    def get_times(self, time_values):
        values = np.zeros((len(time_values), 3)) * self.r.unit
        for ii, epoch in enumerate(time_values):
            rr = self.propagate(epoch).r
            values[ii] = rr
        return values

    def _sample(self, time_values):
        values = self.get_times(time_values)
        return CartesianRepresentation(values, xyz_axis=1)
    ...

The good thing is that it won't break the API as users could just call method Orbit.get_times() to get times. But this won't be as efficient as we would have to compute times in each method call.

[astrojuanlu]

@ritiek notice that you swapped the time values and the position values. Can you give it a second look in case you can apply that idea using the correct code? :)

[ritiek]

@Juanlu001 Yep, my bad. Ignore my previous implementation. How about something like this (and leaving everything else untouched):

class Orbit(object):
    ...
    def sample(self, values=None):
        time_values = self.get_times(values)
        return self._sample(time_values)

    def get_times(self, values=None):
        if values is None:
            return self.get_times(100)

        elif isinstance(values, int):
            if self.ecc < 1:
                nu_values = np.linspace(0, 2 * np.pi, values) * u.rad
            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)

            return self.get_times(nu_values)

        elif hasattr(values, "unit") and values.unit in ('rad', 'deg'):
            values = self._generate_time_values(values)

        return values
    ...

For example (modifying this one):

>>> import matplotlib.pyplot as plt
>>> plt.ion()

>>> from astropy import time
>>> from astropy import units as u

>>> from poliastro.bodies import Earth
>>> from poliastro.twobody import Orbit
>>> from poliastro.plotting import plot


>>> r_p = Earth.R + 165 * u.km
>>> r_a = Earth.R + 215 * u.km

>>> a_parking = (r_p + r_a) / 2
>>> ecc_parking = 1 - r_p / a_parking

>>> parking = Orbit.from_classical(Earth, a_parking, ecc_parking,
                               0 * u.deg, 0 * u.deg, 0 * u.deg, 0 * u.deg,
                               time.Time("2006-01-19", scale='utc'))

>>> print(parking.get_times())
['2006-01-19 00:00:00.000' '2006-01-19 00:00:53.105'
 '2006-01-19 00:01:46.211' '2006-01-19 00:02:39.320'
  ...
 '2006-01-18 23:57:20.680' '2006-01-18 23:58:13.789'
 '2006-01-18 23:59:06.895' '2006-01-19 00:00:00.000']

>>> print(parking.sample())
[( 6543.1366    ,     0.        ,  0.),
 ( 6530.01300033,   414.99453816,  0.),
  ...
 ( 6530.01300033,  -414.99453816,  0.),
 ( 6543.1366    ,     0.        ,  0.)] km

Does this seem right?

[astrojuanlu]

That would be a correct implementation, but I still think Orbit.get_times makes no sense. I'd prefer to return a tuple (times, positions) than have a get_times method. Of course if you want to contribute it, you're invited to do so :)

[astrojuanlu]

Closed in #312.